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

Wrong VertexLayoutBuilder semantics #10

Closed
BenjaminSchaaf opened this issue Sep 29, 2022 · 5 comments · Fixed by #11
Closed

Wrong VertexLayoutBuilder semantics #10

BenjaminSchaaf opened this issue Sep 29, 2022 · 5 comments · Fixed by #11

Comments

@BenjaminSchaaf
Copy link
Contributor

BenjaminSchaaf commented Sep 29, 2022

I encountered some crashes/invalid rendering when using multiple VertexLayoutBuilder. I narrowed this down to VertexLayoutBuilder.end() not being called by me, but in doing so I found numerous issues with VertexLayoutBuilder:

  • begin, add and end mutate the builder but don't take a mutable reference
  • VertexLayoutBuilder is able to be passed to various APIs without end being called. A new type safely encapsulating this requirement would be preferred.
  • begin and add both return &Self, meaning you can't assign their result to a variable. AFAIK Self is idiomatic.

I'm working on a PR for this.

@emoon
Copy link
Owner

emoon commented Sep 29, 2022

Thanks for the report. The way bgfx-rs is done is that the majority of code is auto-generated from this file https://github.com/bkaradzic/bgfx/blob/master/scripts/bgfx.idl using this (right now quite messy/hardcoded) code https://github.com/emoon/bgfx-rs-bindgen/blob/main/src/main.rs

While I will accept a PR for this, I will likely need to re-implement it later on in the generator.

@BenjaminSchaaf
Copy link
Contributor Author

Unfortunately even if I fix the bindgen repo to not use hard-coded paths it's still panicking, and I don't really want to look further into that. I'll fix it in here for now.

@emoon
Copy link
Owner

emoon commented Sep 29, 2022

Unfortunately even if I fix the bindgen repo to not use hard-coded paths it's still panicking, and I don't really want to look further into that. I'll fix it in here for now.

Yes that is fine. I'm planning to remove the hard coding + some cleanup to the code, but I just haven't gotten around to do it

@emoon
Copy link
Owner

emoon commented Oct 7, 2022

@BenjaminSchaaf hey, just FYI 0.15 has been released which includes your changes (and also BGFX updated to latest version)

@BenjaminSchaaf
Copy link
Contributor Author

@emoon Awesome, thanks :)

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 a pull request may close this issue.

2 participants