Reinstate Model Instancing #417

two-one-five · 2020-06-04T04:59:47Z

This reverts commit 86dbf3f, reversing
changes made to d69c895.

This was removed in PR#414 kasenvr/fix/revert-instancing-temporarily.

The above issues outlines the current problem with this PR. This PR will be merged to reinstate instancing once this bug is solved.

From HifiExperiments:

to be completely honest, I have no idea what the bulk of this PR does. I have limited understanding of the serializer code and all the transform stuff is so tangled and insane. for the most part, this PR should exactly match the original merge commit from sabrina, but with the additions outlined below

CORRECTION: The original reversal of changes was to 3592488, not d69c895.

…mporarily-1" This reverts commit 86dbf3f, reversing changes made to d69c895.

vircadia-build-notifier · 2020-06-04T06:37:31Z

The following links are available:
build (macOS-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR417/ProjectAthena-Alpha-PR417-ae0b40b_1850c184.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR417/ProjectAthena-Alpha-PR417-ae0b40b_1850c184.exe

Aitolda · 2020-07-19T18:09:57Z

Not sure I have any answers yet, but after doing some testing with the Pillar.fbx Alezia provided, it appears to import visually at the right size in scale, however in the create tools it's showing as exactly twice it's scale on the x and z axis within interface, while the Y axis (up/down) is acurate to scaled size in blender (which is the Z axis within blender). HOWEVER... in blender this axis is actually the ONLY part that is scaled. So... the non-scaled axis are showing up with incorrect dimensions, while the scaled dimension shows up correctly.

In Blender (2.83) note the scaling and length on the Z axis (Y within interface)
Blender

And note the Y-dimensions within the create menu in interface (again)
Interface

Both remain at 17.8, but the other two dimensions are doubled in interface. Again, they actually are showing up at the correct visual scale for me, just not in the create dimensions.

two-one-five · 2020-08-06T21:00:31Z

Vegaslon noted these two commits may be of interest:
yozlet/interface@ecdca05
yozlet/interface@a70f0f2

HifiExperiments · 2020-08-11T03:26:51Z

libraries/fbx/src/FBXSerializer.cpp

-                glm::mat4_cast(combinedRotation) * joint.postTransform;
+            joint.transform = localTransform * joint.geometricOffset;
+            joint.globalTransform = hfmModel.offset * localTransform * joint.geometricOffset;
+            joint.globalTransformForChildren = hfmModel.offset * localTransform;


this is basically the fix...when hifi did the instancing PR, they removed support for geometricOffset

I don't know if there's an analogous property in GLTF so for now this (globalTransformForChildren) is only used here so that each joint has the correct globalTransform (including geometricOffset), but it doesn't pass geometricOffset down to its children

The top level object in glTF is the "scene". The scene has one or more "nodes". Each node may have a transform plus one or more meshes, and it can have other nodes as children (e.g., to form a skeleton).

HifiExperiments · 2020-08-11T03:43:40Z

libraries/render-utils/src/Model.cpp


-                data.setCauterized(cauterized);
-                data.setRenderWithZones(renderWithZones);


I had two merge conflicts when trying to update this PR to master, which reviewers should pay particular attention to. one was around this area, I think because of the renderWithZones stuff, which I have tried to preserve

HifiExperiments · 2020-08-11T03:44:43Z

libraries/fbx/src/GLTFSerializer.cpp


-                if (indicesAccessorIdx > _file.accessors.size()) {


another was in this file, I think because we added these bound checks

vircadia-build-notifier · 2020-08-11T05:25:47Z

The following links are available:
build (ubuntu-18.04, full)

build (macOS-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR417/Vircadia-Alpha-PR417-a248f48_7bb95a55.dmg

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR417/Vircadia-Alpha-PR417-a248f48_7bb95a55.exe

two-one-five · 2020-08-14T05:46:49Z

While it will be good to review this fully, really all that needs to be considered is what HiFiExperiments added... the rest is stuff High Fidelity kinda added semi-finished but untested fully (how we caught this problem, but maybe they already knew about it and were intending to revisit it, according to the comments, in which case then it was fully considered and of course code-reviewed.)

I know we're dealing with limited resources, so it's my hope that we'll catch more issues in testing. :)

Aitolda · 2020-08-14T06:47:18Z

Tested. Scale and position seems to be working as expected. I did notice that drawcalls don't seem to report accurately, however, after switching back to stable this does not seem to be specific to this PR.

vegaslon · 2020-08-15T12:26:24Z

Understand why they removed model offset from this pr. If you are trying to instance a model having geometry offsets on it will cause all but one of the instanced models to be in the wrong place. We will just have to work around that when the times comes for someone try instancing in a scene.

Aitolda · 2020-08-15T14:52:30Z

Understand why they removed model offset from this pr. If you are trying to instance a model having geometry offsets on it will cause all but one of the instanced models to be in the wrong place. We will just have to work around that when the times comes for someone try instancing in a scene.

You are still having this problem? I did not. I wonder what we may be doing differently.

vegaslon · 2020-08-15T15:04:07Z

No do not have “this” problem as currently stated. I am just saying that may see “new bugs” in future.

HifiExperiments · 2020-08-16T04:09:13Z

@vegaslon the old code (before the original PR removed it) worked by actually altering the underlying vertices of the meshes, which I assume would lead to the problem you mention. but with my new changes, the vertices stay unmodified, but the applied transform changes. I think this should fix the issue, but the only way to be sure would be to create a model with instanced parts and geometric offsets and test it

AleziaKurdis · 2020-08-19T04:00:01Z

Still have the problem with this PR417 build.

AleziaKurdis · 2020-08-19T04:09:30Z

There are links on those model in
#383
if you need this to investigate.

daleglass

Overall this is way too much for me to properly get my head around it in any reasonable amount of time, but I can nitpick a bit and check for anything that jumps out to me.

assignment-client/src/avatars/ScriptableAvatar.cpp

daleglass · 2020-08-31T20:49:12Z

interface/src/raypick/CollisionPick.cpp

@@ -121,8 +121,9 @@ bool CollisionPick::isLoaded() const {
 bool CollisionPick::getShapeInfoReady(const CollisionRegion& pick) {
    if (_mathPick.shouldComputeShapeInfo()) {
        if (_cachedResource && _cachedResource->isLoaded()) {
-            computeShapeInfo(pick, *_mathPick.shapeInfo, _cachedResource);
-            _mathPick.loaded = true;
+            // TODO: Model CollisionPick support


There's a "TODO" here. What does missing this part imply?

daleglass · 2020-08-31T21:06:14Z

libraries/entities-renderer/src/RenderableModelEntityItem.cpp

-        const int MAX_ALLOWED_MESH_COUNT = 1000;
-        if (numMeshes > MAX_ALLOWED_MESH_COUNT) {
-            // too many will cause the deadlock timer to throw...
+        const size_t MAX_VERTICES_PER_STATIC_MESH = 1e6;


Should this be here? Maybe move it to the top?

Also, a million? Perhaps that is a bit much for some of the lower end hardware like the Quest? Perhaps this should even be configurable (even if not actually exposed anywhere yet) so that we can put some other limit for the Quest and such devices?

daleglass · 2020-08-31T22:56:55Z

libraries/fbx/src/GLTFSerializer.cpp

-                    hfmModel.meshExtents.addPoint(transformedVertex);
-                }               
-            }
+    // TODO: Fix skinning and remove this workaround which disables skinning


Another TODO here

Aitolda · 2020-12-10T22:17:05Z

While I need to investigate further, older models that are scaling incorrectly when imported back into current blender 2.9x, resaved without changes, seam to stop having the issue.

AleziaKurdis · 2020-12-10T23:56:03Z

While I need to investigate further, older models that are scaling incorrectly when imported back into current blender 2.9x, resaved without changes, seam to stop having the issue.

I suspected that. If this is the case, it might be a lot of rework, but I think we should go ahead.

digisomni · 2020-12-12T19:35:21Z

We should investigate what the Hub looks like in this PR for example.

digisomni · 2020-12-12T19:38:48Z

Per @ctrlaltdavid: Can we find out the difference between the FBX's and insert a compatibility fix then to check to see if a model was exported with the older version and process that differently?

ctrlaltdavid · 2020-12-12T19:39:07Z

If we can get some example "broken before", "not broken before" and "broken before fixed after" models, it might be possible to see what differences there are in the FBX files.

vircadia-build-notifier · 2020-12-12T21:15:43Z

The following links are available:
build (macOS-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR417/Vircadia-Alpha-PR417-0bc4d1f_21259aa3.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR417/Vircadia-Alpha-PR417-0bc4d1f_21259aa3.exe

AleziaKurdis · 2020-12-18T04:35:02Z

Maybe this could help:

BROKEN:
http://metaverse.bashora.com/objects/ZEDHA/DOORFRAME.fbx
(16 May 2017)

http://metaverse.bashora.com/objects/ZEDHA/FENETRE.fbx
(8 May 2017)

http://metaverse.bashora.com/objects/WOTHAL/PILAR.fbx
(2018)

http://metaverse.bashora.com/objects/WOTHAL/PILAR_RE_EDITED_BL281.fbx
(Re-edited in Blender 2.81 (15 dec 2020), Materials get modified)
but with this version it has the same issue!

NOT BROKEN:
http://metaverse.bashora.com/objects/ZEDHA/Cafe.fbx
(15 May 2017)

http://metaverse.bashora.com/objects/WOTHAL/PILAR_RE_EDITED_BL281_WITH_APPLY.fbx
(Re-edited in Blender 2.81 (15 dec 2020), Materials get modified, AND I did Apply Location, Scale , Rotation)

I clearly suspect that it has to do with the "Apply: Location, Scale , Rotation"
A concept that I wasn't even know 18 months ago...

so depending if some objects has been rotate or scale in "Object Mode" in Blender, without have done "Apply"
these objects has the problem. (in the PR417 version)
But sometime, I was starting to work directly in "Edit Mode" in blender without have to play with it in "Object Mode",
then those model would have resisted.
Of course, all those model are render correctly in the release 2020.3.2 and are display broken in PR417.

AleziaKurdis · 2020-12-18T06:00:30Z

Here, the "PILAX.blend" file with the scale (Z-UP)

In-word, the PILAR.fbx model is rescaled inside its bounding box with opposite proportions.
Those that had the scale of 1,1,1 are untouched. (As observed, being the opposite of them-self)

AleziaKurdis · 2020-12-19T04:00:29Z

This is not a old model or old software issue, but clearly a bug.
I can now reproduce it with model created today with Blender 2.8.1

2020.3.2 - Demeter

PR 417

Here's the step to reproduce in Blender (probably any version)

1- Open Blender

2- Clear the scene and add a mesh of type cube

3- in "Object Mode", select the cube and do R, Z, 45
(This rotate the Object of 45 degree on the Z axis (blender being Z-UP))

4- Switch to "Edit Mode"

5- Click A to select All

6- Do R, Z, -45
(This rotate the Object of -45 degree on the Z axis.)

7- you can add a material and save this.

8- Export as an FBX (you can use the Metaverse Plugin (Menithal), should not be the problem)

9- Import In-world...
Results:
With 2020.3.2, you should get a perfect cube that fits its bounding box when selected.

With PR417, you should get a long horizontal rectangular prism (Y axis being the longest side) and not fitting the bounding box that is a perfect cube.

Additional tests
The second test you see on the pictures (the blue cube)
is when you rotate only it in Object Mode. As you can see, the rotated model is rendered accordingly in it's bounding box, which make sense.

I made also additional test with the scale not being applied, and it's not that, the unique responsible is that double rotation that is badly computed at some point.

Here's the 3 test models:
http://metaverse.bashora.com/objects/broken417/BROKEN_CUBE_ROTATE_OBJECT_ONLY.fbx
http://metaverse.bashora.com/objects/broken417/BROKEN_CUBE_ROTATE_ONLY.fbx
http://metaverse.bashora.com/objects/broken417/GOOD_CUBE.fbx

AleziaKurdis · 2020-12-19T04:23:47Z

Here How it looks inside the FBX of these 3 models:

stale · 2022-05-05T19:02:18Z

Hello! Is this still an issue?

Revert "Merge pull request #414 from kasenvr/fix/revert-instancing-te…

1850c18

…mporarily-1" This reverts commit 86dbf3f, reversing changes made to d69c895.

two-one-five added do not merge do not merge due to issues or pending updates renderer Touches or changes rendering labels Jun 4, 2020

two-one-five assigned HifiExperiments Jun 4, 2020

two-one-five changed the title ~~Reinstate Instancing~~ Reinstate Model Instancing Jun 6, 2020

HifiExperiments added 2 commits August 10, 2020 18:24

merge with master

0c455ea

possible fix for geometricOffset issues

cda77eb

HifiExperiments reviewed Aug 11, 2020

View reviewed changes

fix merge issues

7bb95a5

HifiExperiments reviewed Aug 11, 2020

View reviewed changes

HifiExperiments added bugfix needs CR (code review) needs testing (QA) The PR is ready for testing rebuild rebuild through the GithubActions and removed do not merge do not merge due to issues or pending updates labels Aug 11, 2020

two-one-five requested review from ctrlaltdavid and daleglass August 11, 2020 04:22

daleglass suggested changes Aug 31, 2020

View reviewed changes

digisomni added rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Dec 12, 2020

digisomni added help wanted Extra attention is needed and removed needs CR (code review) needs testing (QA) The PR is ready for testing labels Dec 19, 2020

digisomni modified the milestones: 2020.3.3 Release, 2020.3.4 Release, 2021.1.0 Release Dec 21, 2020

digisomni modified the milestones: 2021.1.0 Release, 2021.1.1 Release Jan 27, 2021

digisomni modified the milestones: 2021.1.1 Eos Release, 2021.1.2 Eos Release Mar 27, 2021

digisomni modified the milestones: 2021.1.2 Eos Release, 2021.2.0 Selene Release May 28, 2021

digisomni modified the milestones: 2021.2.0 Selene Release, 2021.2.1 Selene Release Oct 30, 2021

digisomni added the dormant This PR is on hold but has interest/use surrounding it. label Nov 6, 2021

stale bot added the stale Issue / PR has not had activity label May 5, 2022


		data.setCauterized(cauterized);
		data.setRenderWithZones(renderWithZones);

Reinstate Model Instancing #417

Are you sure you want to change the base?

Reinstate Model Instancing #417

Uh oh!

Conversation

two-one-five commented Jun 4, 2020 • edited by ctrlaltdavid Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vircadia-build-notifier commented Jun 4, 2020

Uh oh!

Aitolda commented Jul 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

two-one-five commented Aug 6, 2020

Uh oh!

HifiExperiments Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

ctrlaltdavid Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

HifiExperiments Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

HifiExperiments Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

vircadia-build-notifier commented Aug 11, 2020

Uh oh!

two-one-five commented Aug 14, 2020

Uh oh!

Aitolda commented Aug 14, 2020

Uh oh!

vegaslon commented Aug 15, 2020

Uh oh!

Aitolda commented Aug 15, 2020

Uh oh!

vegaslon commented Aug 15, 2020

Uh oh!

HifiExperiments commented Aug 16, 2020

Uh oh!

AleziaKurdis commented Aug 19, 2020

Uh oh!

AleziaKurdis commented Aug 19, 2020

Uh oh!

daleglass left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daleglass Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

daleglass Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

daleglass Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

Aitolda commented Dec 10, 2020

Uh oh!

AleziaKurdis commented Dec 10, 2020

Uh oh!

digisomni commented Dec 12, 2020

Uh oh!

digisomni commented Dec 12, 2020

Uh oh!

ctrlaltdavid commented Dec 12, 2020

Uh oh!

vircadia-build-notifier commented Dec 12, 2020

Uh oh!

AleziaKurdis commented Dec 18, 2020

Uh oh!

AleziaKurdis commented Dec 18, 2020

Uh oh!

AleziaKurdis commented Dec 19, 2020

Uh oh!

AleziaKurdis commented Dec 19, 2020

Uh oh!

stale bot commented May 5, 2022

Uh oh!

two-one-five commented Jun 4, 2020 •

edited by ctrlaltdavid

Loading

Aitolda commented Jul 19, 2020 •

edited

Loading