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

cleaning up new id system a bit #2516

Open
mbechard opened this issue Jan 29, 2021 · 3 comments
Open

cleaning up new id system a bit #2516

mbechard opened this issue Jan 29, 2021 · 3 comments

Comments

@mbechard
Copy link
Contributor

mbechard commented Jan 29, 2021

I think the errors that arose from PR #2458 indicate the way the new id system is setup is too error prone to code. Is there any reason why the ID isn't a struct with the id number, level number as separate variables, and have the maps/sets that use the ID be keyed with that struct type instead? It seems like it would be much less error prone than all the bitshift/or/anding that is going on which is easy to forget to do at each usage point. It's not like it's saving any memory.
Or, if we want to stay lower level, a union between a int64_t and a set of bit fields, so it's easier to access the different sections of the int64_t without needing to remember shifting etc when wanting different types of data out of the id.
I'm happy to do the changes if there are no objections.

Thoughts? @johnkslang @ShchchowAMD @greg-lunarg

@ShchchowAMD
Copy link
Contributor

No objections. Current new id process is based on previous one. It is better to create a new struct containing two seperate variables and using interface rather than idshift as you mentioned above. To avoid errors.

I'd love to help testing once you finished.

@greg-lunarg
Copy link
Contributor

Interesting idea. I will be reviewing the code shortly and will have a better opinion then.

@greg-lunarg
Copy link
Contributor

I approve of this idea. #2458 is merged, so work on this issue can proceeed.

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

No branches or pull requests

3 participants