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

Mesh: add thin instances #8208

Merged
merged 17 commits into from
May 18, 2020
Merged

Mesh: add thin instances #8208

merged 17 commits into from
May 18, 2020

Conversation

Popov72
Copy link
Contributor

@Popov72 Popov72 commented May 18, 2020

I took model on the regular instance implementation for the thin instance implementation:

  • added a property _thinInstanceDataStorage of type _ThinInstanceDataStorage to hold the main data (much like _instanceDataStorage / _InstanceDataStorage).
  • created an additional _userThinInstanceBuffersStorage to hold data for additional custom attributes other than the matrices (much like _userInstancedBuffersStorage). This way, if no custom attributes are added, no penalty paid

I have prefixed all methods by thinInstance, I can change that (notably, addThinInstance is thinInstanceAdd instead).

By default, the bounding box is recomputed to encompass all thin instances, but it can be disabled by setting doNotSyncBoundingInfo to true (already existing property).

As discussed, thin instances are always displayed with shader code assuming non uniform scaling, but with an optimized code (shearing not supported).

@sebavan sebavan requested review from bghgary and deltakosh May 18, 2020 15:15
@Popov72
Copy link
Contributor Author

Popov72 commented May 18, 2020

Hum, shouldn't I add an optional parameter to the thinInstanceSetBuffer method to create a static buffer instead of a dynamic one, in case the user know they won't update the buffer?

@sebavan
Copy link
Member

sebavan commented May 18, 2020

sounds totally fine to me :-)

@sebavan
Copy link
Member

sebavan commented May 18, 2020

Also should try to ensure we use the types that are not creating dupplicate in the engine updateBuffer ???

Copy link
Contributor

@deltakosh deltakosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALLL GOOD. You rock

Can you add a visual test for thinInstances?

@Popov72
Copy link
Contributor Author

Popov72 commented May 18, 2020

I also wonder if we shouldn't also let the user be able to set the current number of instances (but always < matrixBuffer.length / 16): that way, they would be able to allocate a bigger buffer at start in case they need to have more instances during the course of the program, they would not have to reallocate / copy a buffer each time they have to add an instance.

@Popov72
Copy link
Contributor Author

Popov72 commented May 18, 2020

Visual test added

@Popov72 Popov72 marked this pull request as ready for review May 18, 2020 17:27
@deltakosh deltakosh merged commit 5a6595a into BabylonJS:master May 18, 2020
@Popov72 Popov72 deleted the mesh-thininstance branch July 15, 2020 20:04
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.

3 participants