Releaseable point cloud #24
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trym-b
force-pushed
the
point-cloud-class
branch
3 times, most recently
from
afd9bb0 to
7b3eb09
Compare
nedrebo
reviewed
Nov 11, 2019
trym-b
force-pushed
the
point-cloud-class
branch
2 times, most recently
from
6b1e370 to
d28a15a
Compare
Instead of transforming the point cloud directly to a numpy buffer, we should instead return a intermediate layer, since in the underlying C++ API point cloud exposes various functions such as width and height. Making buffer releasable also created some technical debt: assertNotReleased is needed since accessing the underlying a C++ object after it has been released causes a crash. Ignored flake8's D402, since it mistakenly thought that the paranthesis in the docstring was the signature of the function.
trym-b
force-pushed
the
point-cloud-class
branch
from
d28a15a to
92d185e
Compare
|
Ready for next round. |
nedrebo
approved these changes
Nov 12, 2019
| """Release the underlying resources.""" | ||
| self.__impl.release() | ||
|
|
||
| def __enter__(self): |
There was a problem hiding this comment.
Maybe we should have a mixin or base class or something for these three functions. They are duplicated many places now.
You are just adding to the existing solution, so no need to fix in this PR. But if you agree please add an issue for it.
There was a problem hiding this comment.
Could probably be done through a decorator:
@utils.context_manager
class SomethingThatRequiresContextManager:
....
Will create an issue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of transforming the point cloud directly to a numpy buffer, we
should instead return a intermediate layer, since in the underlying C++
API point cloud exposes various functions such as width and height.
Making buffer releasable also created some technical debt:
assertNotReleased is needed since accessing the underlying a C++ object
after it has been released causes a crash.