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

Fix big markers #116

Merged
merged 3 commits into from
Sep 26, 2019
Merged

Fix big markers #116

merged 3 commits into from
Sep 26, 2019

Conversation

griwodz
Copy link
Member

@griwodz griwodz commented Sep 25, 2019

With this change, I feel that it's easier to understand how layers map to collections, and I can find 4 markers in the large sample image that gave you trouble.

@griwodz griwodz self-assigned this Sep 25, 2019
@griwodz
Copy link
Member Author

griwodz commented Sep 25, 2019

Why yet another unique_ptr, this time inside a vector? Am I stupid?

Well, EdgePointCollection has no copy constructor, it is forbidden with =delete for a good reason: it contains lots of unique_ptrs to arrays, and copy constructors mess things up because they seem to create identical copies, while they are in reality invalidting the source object.
Forbidding the copy constructor means that vEdgePointCollections cannot be of type vector<> because vector<> leads to compile errors when the copy constructor of its type does not exist.
list<>, on the other hand, was the reason why we got into this mess in the first place. Since we cannot use operator[] on list<>, we (probably I) created this bug where edges were not stored in the right layer.

So I tried vector<EdgePointCollection*> first, and that gave the correct result for me as well. But the host code is full of exception raising and exception checking stuff, and that would probably lead to memory leaks.

vector<unique_ptr > is a bit uglier, but it calls delete on the EdgePointCollection objects when leaving the context, even in case of an exception.

@griwodz
Copy link
Member Author

griwodz commented Sep 25, 2019

The other image, where a single large marker in the center was missing, works for me as well.

@fabiencastan
Copy link
Member

Maybe we could use boost::ptr_vector which is done for this use case?

unique_ptr to prevent memory leak in case of an exception

put markers into fixed layer
@fabiencastan fabiencastan merged commit cc30a2d into develop Sep 26, 2019
@fabiencastan fabiencastan deleted the fix-BigMarkers branch September 26, 2019 17:16
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