-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Allow changing LUT properties of a shape actor #1668
Allow changing LUT properties of a shape actor #1668
Conversation
@VictorLamoine , can you take a look? |
Yes but not until 4 days! |
Sounds good! |
This works great and the code looks clean. 👍 |
Great, thanks for reviewing the code!
There are two issues I want to address first:
|
@SergioRAgostinho Actually I believe that this PR is ready to merge. I will add a tutorial in a separate PR once all of the pending changes to the visualizer are complete. |
This is targeted for 1.9.0, so we won't be merging this till the 1.8.1 probation period is over. |
I see, in that case would it make sense to change the label to "ready to merge"? |
* \param[in] val1 the first value to be set | ||
* \param[in] val2 the second value to be set | ||
* \param[in] id the shape object id | ||
* \param[in] viewport the view port where the shape's properties should be modified (default: all) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I see, this parameter is not used at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean viewport
? Yes, it is not used in any of the setShapeRenderingProperties
or 'setPointCloudRenderingProperties' functions. I am not sure why it is there in the first place. Should we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it left there to allow (in the future) individual control over which viewport those properties are applied?
But to be honest that's not the only thing that bothers me with the signatures. The int property
should be strong typed to the corresponding enum, like RenderingProperties
for instance. Just by reading the function signature description and help, it's impossible to know which properties are accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Viewport is in pretty much all the functions but sometimes it's not implemented, which is is bad because it lures the user.
Changing the int
property
should be done in an other PR in my opinion because it must be done everywhere in PCLVisualizer code (including interactor_style.cpp
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand that it seems to be a convention to have a viewport parameter even if we don't really need it, but I think the documentation should be clear about that it is not used.
Yep, centralized int
-> enum
conversion would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can work on the int
-> enum
conversion. I have created an issue for that #1692. I will work on it after this PR is merged.
The tutorial you're gonna submit, will it be a single tutorial covering all the rendering properties related PRs you've created in the past weeks/months or is it one tutorial per each? In the latter case I would say to append it as a commit into this PR. In the former, this is indeed ready to ship. |
I was thinking of a single tutorial showing all the capabilities of the visualizer and a nice code example showing how to use them (similar to the example code with this PR). |
This PR is a follow up to #1581. It allows changing the properties of a LUT table used for visualizing a shape actor using the
setShapeRenderingProperties
function. The functionality added mimics that ofsetPointCloudRenderingProperties
.Here is a minimal example code.