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

fix mesh allocation bug and public mesh api improvements #768

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

cart
Copy link
Member

@cart cart commented Nov 2, 2020

Fixes #764

This removes attribute_buffer_descriptor_reference from Mesh in favor of computing it on demand. This prevents the "mesh asset change event" from triggering .

I also refactored some things and encapsulated the Mesh fields in favor of accessor methods. This allows us to cut down on user facing boilerplate and makes the backing attribute hashmap an implementation detail.

@julhe I also removed the sorting to cut down on the cost of computing VertexBufferDescriptor. I know I asked you to sort, but now that we're computing the descriptor more frequently its worth optimizing.

Long term I think we should avoid re-computing the VertexBufferDescriptor. Instead we should adopt something like this:

  1. whenever a mesh changes (asset change event), calculate the new vertexbufferdescriptor, hash it, and store a mapping between the hash and the descriptor as a resource. also store a mapping between the Mesh and the Hash
  2. Instead of storing a copy of the VertexBufferDescriptor in every Entity's RenderPipeline::specialization (which is allocation heavy), store the hash instead.
  3. In PipelineCompiler, look up the VertexBufferDescriptor using the specialization's vertexbufferdescriptor hash

@Moxinilian Moxinilian added C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change A-Rendering Drawing game state to the screen labels Nov 2, 2020
@julhe
Copy link
Contributor

julhe commented Nov 2, 2020

Yup, sounds good to me.

Although, I would like not to encapsulate the attributes (or any other fields), unless necessary. But for as a workaround for 0.3, I guess thats ok. 🙂

@cart cart merged commit 44b3e24 into bevyengine:master Nov 2, 2020
@cart
Copy link
Member Author

cart commented Nov 2, 2020

Yeah I like to keep fields "open" by default too, especially when it improves ergonomics / utility. But in this case I think its actually more ergonomic to encapsulate fields (because we can auto-convert). And in this case I think the methods help guide people toward doing the "right thing".

Its definitely a matter of opinion and this one is definitely less clear cut than others.

@J-F-Liu
Copy link
Contributor

J-F-Liu commented Nov 3, 2020

Mesh:: attribute(&mut self, should be Mesh:: attribute(&self.

@cart
Copy link
Member Author

cart commented Nov 3, 2020

nice catch

@cart
Copy link
Member Author

cart commented Nov 3, 2020

#774

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a lot of new buffers created all the time after #599
4 participants