Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add contour angle to contours report #915

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SamCarlberg
Copy link
Member

Angle is from minAreaRect, and is in the closed range [-90, 0]
Rework contours operations to be a bit more functional
Make filter contours a bit more optimized

Add a LazyInit class for lazily creating objects, instead of using a bizarre pattern with Optional
Add a utility class for streaming Java-fied std::vector types such as MatVector as Java Stream objects. This currently just supports MatVector, but can be added to in the future.

*
* @param <T> the type of held data
*/
public class LazyInit<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there are no thread safety guarantees offered? Is this intentional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping!

@JLLeitschuh
Copy link
Member

Just want to make sure we're not unintentionally introducing any memory leaks with this logic. Are there any native pointers that are created in these new stream operations that need to be cleaned up?

@SamCarlberg
Copy link
Member Author

The stream operation itself doesn't allocate anything - it's just standard indexed iteration. A new RotatedRect array is created for each contour report object, but the impact in native memory is minimal and the garbage collector can easily handle it.

@JLLeitschuh
Copy link
Member

Reminder, the garbage collector has no idea how large native objects truly are, so it can leave them around for far longer than you think it will.

If there is a clean place to de-allocate them after usage, please do so. Otherwise, it's fine.

@SamCarlberg
Copy link
Member Author

Right. The C++ class is small (only holds a few numbers), so the memory impact is low. We also prompt the garbage collector every few seconds so I'm not too worried

Angle is from minAreaRect, and is in the range [-90, 0)
Rework contours operations to be a bit more functional
Make filter contours a bit more optimized
@SamCarlberg SamCarlberg force-pushed the publish-contour-angles branch from d870e8b to 5efa91e Compare April 18, 2019 23:08
@codecov-io
Copy link

Codecov Report

Merging #915 into master will increase coverage by 0.11%.
The diff coverage is 29.09%.

@@             Coverage Diff              @@
##             master     #915      +/-   ##
============================================
+ Coverage     52.86%   52.98%   +0.11%     
  Complexity        1        1              
============================================
  Files           327      329       +2     
  Lines          8852     8869      +17     
  Branches        560      555       -5     
============================================
+ Hits           4680     4699      +19     
+ Misses         3969     3967       -2     
  Partials        203      203

*
* @param factory the factory to use to create the held value
*/
public LazyInit(Supplier<? extends T> factory) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't make this expose a constructor, it doesn't tightly couple this to needing to be newed.

*/
public void clear() {
value = null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should you be able to clear a lazy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants