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

GenericVBO changes #25

Merged
merged 1 commit into from
Jan 28, 2019
Merged

Conversation

TheAIBot
Copy link
Collaborator

@TheAIBot TheAIBot commented Jan 28, 2019

Part 1 of 3 from PR #24.

Add new GenericVBO constructor.
Generic VAOs uses the new constructor.
Moved IGenericVBO into the OpenGL namespace.

There is a few more changes i would like to make but i want to hear your opinion first. I want to move GenericVBO into the OpenGL namespace as well. If the user has to use it then it should probably be there.

I would also like to change the name of GenericVBO. The only difference between VBO and GenericVBO is that GenericVBO contains a name. So i would like to change the name of GenericVBO and IGenericVBO to something like NamedVBO and INamedVBO. Both VBO and GenericVBO are generic so having generic in the name isn't very descriptive. Afaik i don't think this change would break much as GenericVBO was only recently added.

@giawa giawa merged commit 59b1320 into giawa:dotnetcore Jan 28, 2019
@giawa
Copy link
Owner

giawa commented Jan 28, 2019

Looks good, thanks! Since you've broken IGenericVBO into its own file I would suggest doing this with IGenericVAO, etc (if relevant to future PRs). I also noticed the Name property could use an auto property. Since we've been embracing auto properties I think it's worth getting that in there. What do you think?

@giawa
Copy link
Owner

giawa commented Jan 28, 2019

After the #3 PR has been merged I'll go through and try to separate classes into their own files instead of having several class definitions in a single file. So no worries if any get missed during these PRs. It's easy to take care of later.

@TheAIBot TheAIBot deleted the add-GenericVBO-constructor branch January 31, 2019 08:22
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.

None yet

2 participants