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 user data support to CompositeInstruction #383

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

Levi-Armstrong
Copy link
Contributor

No description provided.

@marip8
Copy link
Contributor

marip8 commented Sep 25, 2023

What is the intended purpose of user data in the composite instruction?

@Levi-Armstrong
Copy link
Contributor Author

What is the intended purpose of user data in the composite instruction?

The composite instruction is not a poly type so this allow users to store additional data with the program if needed.

@Levi-Armstrong Levi-Armstrong merged commit 1a19448 into master Sep 26, 2023
8 of 10 checks passed
@Levi-Armstrong Levi-Armstrong deleted the feat/AddUserDataCompositeInstruction branch September 26, 2023 17:08
@marip8
Copy link
Contributor

marip8 commented Sep 26, 2023

Did you have any specific ideas of what types of data might be put here? I worry that just adding extra members to these classes to support any possible use-case will end up bloating them and making it hard to understand what purpose the members are supposed to serve

@Levi-Armstrong
Copy link
Contributor Author

This is to prevent such a thing because the user data allows you to contain any additional data you may need to embed with the program without having to add anything additional to the composite instruction class. I don't understand why this would be hard for members to understand the purpose since the name is self explanatory (user data) and the use case is up to the user.

@Levi-Armstrong
Copy link
Contributor Author

Levi-Armstrong commented Sep 27, 2023

Also this is common practice, which can found in ogre, ogre2, gz-rendering, etc.

https://github.com/gazebosim/gz-rendering/blob/61bf635a5b6daf4918f37f837d0a5076adc5c879/include/gz/rendering/Node.hh#L37-L47

@marip8
Copy link
Contributor

marip8 commented Sep 27, 2023

That's fine; I just wanted to caution against adding arbitrary-looking fields to classes to accommodate theoretical use-cases. It's easy to just keep adding members to classes to support new use-cases rather than thinking harder about a potentially better solution that uses the existing framework

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

Successfully merging this pull request may close these issues.

2 participants