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

Use strongly typed 'enum' instead of 'int' for properties in PCLVisualizer code #1692

Open
aecins opened this issue Aug 24, 2016 · 11 comments · May be fixed by #5526
Open

Use strongly typed 'enum' instead of 'int' for properties in PCLVisualizer code #1692

aecins opened this issue Aug 24, 2016 · 11 comments · May be fixed by #5526
Labels
effort: low Rough estimate of time needed to fix/implement/solve good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue module: visualization

Comments

@aecins
Copy link
Contributor

aecins commented Aug 24, 2016

Right now functions like setPointCloudRenderingProperties use an int to represent the property to be modified. This means that there is no way of knowing which properties are accepted by the function. A solution is to use strongly typed enum instead of int for properties. The issue was brought up in #1668.

@StephenNneji
Copy link
Contributor

I could take this issue if its still relevant considering its been dormant since 2016.

@SergioRAgostinho
Copy link
Member

That would be great I would say. Just self assign yourself to this issue to help us keep track of things.

@StephenNneji
Copy link
Contributor

@SergioRAgostinho I don't have write access so I cannot assign this to myself

@SergioRAgostinho
Copy link
Member

You're right. In this case the only way of assigning a non member of the organization to the issue is if that person is the one responsible for opening the issue. In this case, we can't do much else.

@SergioRAgostinho SergioRAgostinho removed this from the pcl-1.9.0 milestone Jan 26, 2018
@stale
Copy link

stale bot commented May 19, 2020

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the status: stale label May 19, 2020
@kunaltyagi kunaltyagi added effort: low Rough estimate of time needed to fix/implement/solve good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue module: visualization labels May 20, 2020
@stale stale bot removed the status: stale label May 20, 2020
@theoniko
Copy link
Contributor

theoniko commented Jan 7, 2022

Hello,
Could i work on this one? Is just using enum class instead of using ints in function singatures?

@mvieth
Copy link
Member

mvieth commented Jan 8, 2022

Hello, Could i work on this one?

Sure, that would be great

Is just using enum class instead of using ints in function singatures?

On a quick look, I saw that there are already enums defined here, but each function only accepts some of those values. So maybe it makes sense to specify in the documentation of each function, which properties are accepted? Feel free to propose something, or maybe @larshg has an idea

@theoniko
Copy link
Contributor

theoniko commented Nov 11, 2022

I thought adding an assert or [[deprecated("reason")]] for property but it is already handled with corresponding pcl::console::print_error. I think updating the documentation of setPointCloudRenderingProperties and maybe turning RenderingProperties to enum class are enough.

Also, viewport is never used in any setPointCloudRenderingProperties and the setters in my eyes shall return [[noreturn]] bool or void.

@mvieth
Copy link
Member

mvieth commented Nov 12, 2022

Feel free to create a pull request with your proposed changes, then we can discuss them in detail 👍

@theoniko
Copy link
Contributor

theoniko commented Dec 3, 2022

Feel free to create a pull request with your proposed changes, then we can discuss them in detail 👍

I created an initial PR. Please feel free to add your suggestions.

@theoniko
Copy link
Contributor

@mvieth: Could you please have a look to PR 5526?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: low Rough estimate of time needed to fix/implement/solve good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue module: visualization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants