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 thinInstanceCount setter to work with mesh clone #12434

Merged
merged 3 commits into from
Apr 25, 2022

Conversation

BlakeOne
Copy link
Contributor

@BlakeOne BlakeOne commented Apr 25, 2022

After cloning a mesh that has thin instances, setting thinInstanceCount on the clone currently doesn't work (unless it is set to 0 the setter will return without updating the internal instanceCount).

With this PR, if a mesh doesn't have the thin instance matrixData, then it checks to see if the mesh's source has it before defaulting the max instance count to 0.

PG Repro: https://playground.babylonjs.com/#217750#76
Forum: https://forum.babylonjs.com/t/cant-set-thininstancecount-for-clone/29674

@azure-pipelines
Copy link

Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags.

@BlakeOne BlakeOne marked this pull request as draft April 25, 2022 05:35
@azure-pipelines
Copy link

Snapshot stored with reference name:
refs/pull/12434/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12434/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12434/merge/index.html#WGZLGJ#4600

To test the snapshot in the playground itself use (for example):

https://playground.babylonjs.com/?snapshot=refs/pull/12434/merge#BCU1XR#0

@BlakeOne BlakeOne changed the title Fix thinInstanceCount to work with mesh clone Fix thinInstanceCount setter to work with mesh clone Apr 25, 2022
@azure-pipelines
Copy link

Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags.

@BlakeOne BlakeOne marked this pull request as ready for review April 25, 2022 06:29
@azure-pipelines
Copy link

Snapshot stored with reference name:
refs/pull/12434/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12434/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12434/merge/index.html#WGZLGJ#4600

To test the snapshot in the playground itself use (for example):

https://playground.babylonjs.com/?snapshot=refs/pull/12434/merge#BCU1XR#0

@azure-pipelines
Copy link

Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags.

@azure-pipelines
Copy link

Snapshot stored with reference name:
refs/pull/12434/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12434/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12434/merge/index.html#WGZLGJ#4600

To test the snapshot in the playground itself use (for example):

https://playground.babylonjs.com/?snapshot=refs/pull/12434/merge#BCU1XR#0

@Popov72
Copy link
Contributor

Popov72 commented Apr 25, 2022

This is a breaking change, as now the cloned mesh will be displayed with the same thin instances than the source mesh.

But it does not seem unreasonable to me to expect this behaviour when cloning a mesh with thin instances. And if we don't want the thin instances in the cloned mesh, we can call makeGeometryUnique on the mesh.

Let's see what the Babylon.js team think about this.

@BlakeOne
Copy link
Contributor Author

BlakeOne commented Apr 25, 2022

This is a breaking change, as now the cloned mesh will be displayed with the same thin instances than the source mesh.

It won't be thou unless you explicitly set the clone's thinInstanceCount to the number of thin instances that you want displayed for it. Otherwise the behavior should be the same as it is now AFAIK.

Because the only change after the PR should be that E.G. when you set it to 3 on the clone, you get the first 3 thin instances displayed for it as expected, if you set it to 2 you get just the first 2 as expected, etc, for the clone. Currently nothing would happen at all (the setter just returns), which seems pretty unexpected IMO...

@sebavan sebavan merged commit 4dcbe5b into BabylonJS:master Apr 25, 2022
@BlakeOne BlakeOne deleted the bug-thinInstanceCount-setter branch April 25, 2022 18: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