Model Entities: useOriginalPivot Property #951

HifiExperiments · 2021-01-02T07:52:28Z

implements #284 resolves #793

adds a new property to Model Entities: useOriginalPivot

defaults to false (current behavior), but adds option to toggle in new model dialog

caveat: in create, the bounding box of a selected entity is centered around the actual entity position, which is now offset from the model's position when useOriginalPivot is true. I actually like this, as it reflects that offset, but maybe others have different opinions

Test plan:

run this script: https://gist.github.com/HifiExperiments/7b3faa7919eed7a308c762ffe03d893f/raw/615f87b8315e433ac35b23e47a52ab6954e27f6b/PivotTest.js
first and third models have useOriginalPivot false, second and fourth are true. First and second models use natural dimensions, third and fourth use { 1, 1, 2 }
enable Developer -> Physics -> Show Bullet Collisions. green collision lines should match the 4 models
hover over the entities with your mouse. a small white sphere will appear when your mouse is over the entities. make sure it lines up with the models
press "o" to toggle through different registrationPoints for all of the models. note that they show the correct behavior, and that the physics and ray picking continue to work on all 4 models
verify the workflow of adding a model with/without "Use Original Pivot" via create, and that you can toggle it on a created model via the property window
run luci.js and use the Bounding Box section to confirm that the render bounds of each model is in the correct place, encompassing it regardless of pivot

HifiExperiments · 2021-01-02T07:52:59Z

interface/src/ui/Stats.h

@@ -248,9 +248,9 @@ private: \
 *     <em>Read-only.</em>
 * @property {string} lodStatus - Description of the current LOD.
 *     <em>Read-only.</em>
- * @property {string} numEntityUpdates - The number of entity updates that happened last frame.
+ * @property {number} numEntityUpdates - The number of entity updates that happened last frame.


this (and some below) is unrelated cleanup to silence some warnings and fixup this JSDoc

HifiExperiments · 2021-01-02T07:53:57Z

libraries/entities-renderer/src/RenderableModelEntityItem.cpp

        updateRenderItems = true;
-        model->scaleToFit();


I don't think we need to call scaleToFit here because we call simulate below which calls scaleToFit as necessary

HifiExperiments · 2021-01-02T07:55:24Z

libraries/entities-renderer/src/RenderableModelEntityItem.cpp

@@ -452,7 +465,7 @@ void RenderableModelEntityItem::computeShapeInfo(ShapeInfo& shapeInfo) {
                pointCollection[i][j] = scaleToFit * (pointCollection[i][j] + model->getOffset()) - registrationOffset;
            }
        }
-        shapeInfo.setParams(type, 0.5f * extents, getCompoundShapeURL());
+        shapeInfo.setParams(type, 0.5f * extents, getCompoundShapeURL() + model->getSnapModelToRegistrationPoint());


this is a hack to make sure that the same model with/without useOriginalPivot know that they have slightly different physics shapes (the difference being the offset)

HifiExperiments · 2021-01-02T07:56:06Z

libraries/entities-renderer/src/RenderablePolyVoxEntityItem.cpp

@@ -723,8 +723,10 @@ ShapeType RenderablePolyVoxEntityItem::getShapeType() const {
 }

 void RenderablePolyVoxEntityItem::setRegistrationPoint(const glm::vec3& value) {
-    if (value != _registrationPoint) {


there's some cleanup below to make sure this stuff is all threadsafe and takes into account the pivot

digisomni · 2021-01-02T08:13:17Z

libraries/entities/src/EntityItemProperties.h

@@ -302,6 +302,7 @@ class EntityItemProperties {
    DEFINE_PROPERTY(PROP_RELAY_PARENT_JOINTS, RelayParentJoints, relayParentJoints, bool, ENTITY_ITEM_DEFAULT_RELAY_PARENT_JOINTS);
    DEFINE_PROPERTY_REF(PROP_GROUP_CULLED, GroupCulled, groupCulled, bool, false);
    DEFINE_PROPERTY_REF(PROP_BLENDSHAPE_COEFFICIENTS, BlendshapeCoefficients, blendshapeCoefficients, QString, "");
+    DEFINE_PROPERTY_REF(PROP_USE_ORIGINAL_PIVOT, UseOriginalPivot, useOriginalPivot, bool, false);


Suggested change

DEFINE_PROPERTY_REF(PROP_USE_ORIGINAL_PIVOT, UseOriginalPivot, useOriginalPivot, bool, false);

DEFINE_PROPERTY_REF(PROP_USE_ORIGINAL_PIVOT, UseOriginalPivot, useOriginalPivot, bool, true);

If using the original pivot is the standard, we should probably respect that and encourage doing the same going forward. What you export from Blender should generally show up in the same fashion in-world if at all possible.

Though, is it set to false because all existing entities without this property will get populated with false (in order to prevent breakage?)

In that case, I guess it'll have to stay false, the Create app will have to set it to "true" by default, and then we'll have to plan to set the default value to true at some point further down the line...

yeah, the default needs to be false because of old content. I can change the default in the new model dialog to be true, but there’s no easy way to change the default in the future (without breaking lots of stuff).

Does the domain server not have a way to parse the entity tree and upgrade them to latest? Therefore if an old entity is parsed, set it to false, and then we can set it to true by default in all places for new entities.

we have a way to do this for domain content/backups, so a domain that’s upgrading to a new version could properly detect it, but there’s no way to do it for things like scripts that could be doing anything

vircadia-build-notifier · 2021-01-02T09:31:56Z

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/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-a55be64_20e0c241.dmg

build (ubuntu-18.04, android)

vircadia-build-notifier · 2021-01-03T04:58:15Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-100ee97_5dc45e89.dmg

build (ubuntu-18.04, full)

build (ubuntu-18.04, android)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-100ee97_5dc45e89.exe

libraries/render-utils/src/Model.cpp

libraries/entities/src/EntityItemProperties.cpp

vircadia-build-notifier · 2021-01-04T06:44:13Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-1467eb9_030524c3.dmg

build (ubuntu-18.04, android)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-1467eb9_030524c3.exe

vircadia-build-notifier · 2021-01-04T13:00:26Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-1467eb9_030524c3.dmg

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-1467eb9_030524c3.exe

build (ubuntu-18.04, android)

vircadia-build-notifier · 2021-01-04T19:42:40Z

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/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-1467eb9_030524c3.dmg

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-1467eb9_030524c3.exe

build (ubuntu-18.04, android)

libraries/entities/src/EntityItemProperties.cpp

Aitolda · 2021-01-09T20:36:35Z

Works as intended if you use when of the generated collisions types like basic, exact, etc.. BUT if you use something like Box or Sphere collision center will be the original position. Not necessarily a bug, but this might lead to some confusion. Not sure how we should address this. Maybe if the checkbox is ticked, it does some sort of "get center" from the mesh and puts the shape collision there.

EDIT: Compound shape has the same issue, even if using the same offset model. So originalpivot is not being respected on compound shapes.

Exact
Compound Shape
Box
Test Model

…lPivot=true

HifiExperiments · 2021-01-22T05:07:03Z

@Aitolda thanks for catching that, I think it should be fixed in the latest commit, but of course windows builds are broken here too

vircadia-build-notifier · 2021-01-22T06:12:45Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-01de369_7bed9b07.dmg

build (ubuntu-18.04, full)

vircadia-build-notifier · 2021-01-22T07:39:04Z

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/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-01de369_7bed9b07.dmg

build (ubuntu-18.04, android)

vircadia-build-notifier · 2021-01-23T00:03:51Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-01de369_7bed9b07.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-01de369_7bed9b07.exe

build (ubuntu-18.04, android)

Aitolda · 2021-01-23T06:01:55Z

Pivot point of compound shapes still doesn't seem to be working, at least on the offset cube model I've been using.

Load the Offset Cube
Set shape type to compound
paste the same model in the compound shape URL
switch pivot type and test
rinse and repeat

Offset Cube

…d shapes

HifiExperiments · 2021-02-04T01:37:33Z

@Aitolda @digisomni ok hopefully NOW it's working 😛

vircadia-build-notifier · 2021-02-04T03:11:57Z

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/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-1510322_ace6bf6c.dmg

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-Alpha-PR951-1510322_ace6bf6c.exe

build (ubuntu-18.04, android)

Aitolda · 2021-02-04T06:23:17Z

Still some major issues with using compound collisions, I see two problems potentially working together.

on an fbx model (only) the compound collision hull is rotated on the wrong axis.
glb
fbx
on both fbx and glb, if the model pivot point is moved away from 0,0,0 within a 3d application before exporting, the compound collision may be ignoring this offset while the main mesh respects it

I've attached several models which should help demonstrate the issues. Unfortunately html doesn't like my naming scheme so I will label them here. Note, in blender Z is up, and the coordinates in my naming scheme reflects this.

ArrowUp(0,0,0)Blender
ArrowUp(0,0,0).fbx
ArrowUp(0,0,0).glb

ArrowUp(-5,-5,0)Blender
ArrowUp(-5,-5,0).fbx
ArrowUp(-5,-5,0).glb

Aitolda · 2021-02-04T06:48:44Z

Thinking about it more, I'm not sure respecting the 0,0,0 (as mentioned in the post above) is even desirable. it would make sense only if you were making complex environments most of the time, and I think a json exported is a better approach for this. Just want to clarify we're talking about the distance of the pivot point from zero in blender or 3dmax, not the pivot point itself, which is the whole point of this pr. In other words, I think with the exception of the wrong rotation on fbx, the compound shape is doing what it's supposed to, and it's the mesh that's incorrectly respecting the distance from 0

Aitolda · 2021-02-05T18:42:24Z

Thinking about it more, I'm not sure respecting the 0,0,0 (as mentioned in the post above) is even desirable. it would make sense only if you were making complex environments most of the time, and I think a json exported is a better approach for this. Just want to clarify we're talking about the distance of the pivot point from zero in blender or 3dmax, not the pivot point itself, which is the whole point of this pr. In other words, I think with the exception of the wrong rotation on fbx, the compound shape is doing what it's supposed to, and it's the mesh that's incorrectly respecting the distance from 0

Regarding treating 0,0,0 in blender/Maya/3dMax export as the pivot point, after doing some digging around, it unfortunately looks like even Unreal and Unity have adopted this. I assume their is some reason, but I think it's one of those things in the grand scheme are actually more difficult on artists who want to bring in elaborate scenes, but perhaps we can tackle that issue by updating the blender plugin. Either way I think once the rotation issue is solved on fbx we should move forward as is for the time being.

HifiExperiments · 2021-02-06T04:51:58Z

after a conversation with @Aitolda, he found that other engines also have problems with non-zero pivot points (https://forum.unity.com/threads/wrong-pivot-point-from-blender-to-unity.423176/). for now, we think it's ok to move forward with this, and if we fix it in the future, we can try to do so in a backwards-compatible way. after this is merged, I'll open an issue tracking that

digisomni · 2021-02-06T05:02:56Z

Okeydokeseys!

vircadia-build-notifier · 2021-02-06T06:37:29Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-PR951-6750330_766b91f1.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR951/Vircadia-PR951-6750330_766b91f1.exe

build (ubuntu-18.04, android)

AleziaKurdis · 2021-02-10T03:52:46Z

Just tested this. (I have not read any of the comments in this thread yet)
Here are my impressions:

The feature works as expected. I tested it with fbx and glb

But where it hurts is about having this behavior as default value in the UI.
It looks like this with an ex-centric pivot:

I don't know how other tools deal with this but is the bounding box is clearly not design to support that.
This is also ruins a bit the "snap to surface" feature. Is there a way to figure where is the edge of the model anymore?
A lot of stuff are going to behave weirdly if we have this by default.

I think we should try to figure if honoring the original pivot is the mainstream usecase before decide if this needs to be the default option. What are the usecase where you need more the original pivot than a consistent central point?

In my example the pivot was largely outside. BI suspect that in many cases, people will just have it approximatively in the middle, but without being mathematically in the center. So they might get the bounding box offset in a significant number of cases.
I would have let this option for the case where it is intentional (not as default). This is my opinion.

In the create app (Create Model), the option is not persisted using a setting like what we have for "grabbable".
You can be sure that I will minimally plug such a setting in the future to not have this defaulted (minimally for my-self).
I see advantage for this feature for doors, or anything that we need an ex-centric pivot. Which is not the main usecase for me.

I tested the object as dynamic... here too, excentricity doesn't appears to be a main desired behavior.

To recap:

Feature is good
Set by Default in UI: Not sure about this. (I assume it is not by the api, because that could ruin scripts)
Set by Default in UI: create app behave weirdly which is not going to reflect stability for new comers.
A setting in create app would be a must. (Consider to log an issue for this)

HifiExperiments · 2021-02-10T06:45:34Z

@AleziaKurdis like I said in the description, I actually think the bounding box thing in create is somewhat helpful, as it reflects the ACTUAL position of the entity, but I agree it’s another layer of complexity. I’m open to suggestions on how to fix this...we could expose the offset to create and draw two different boxes maybe?

snap to surface should still work, since it uses findRayIntersection, right? that should still work, even on pivoted models.

I agree the setting should be persistent!

Kalila wanted to get this in for the next release but I can sync with you to fix any bugs you find or make any improvements in a follow up

in general, I think this feature request is less about eccentricity for rotations and more about making building complex scenes easier for modelers

HifiExperiments added 4 commits December 16, 2020 21:01

working on pivot

7069e48

Merge remote-tracking branch 'upstream/master' into pivot

f33c7de

wip

1475cae

finishing touches

002271a

HifiExperiments commented Jan 2, 2021

View reviewed changes

cleanup + create

20e0c24

HifiExperiments force-pushed the pivot branch from 5cf7fe9 to 20e0c24 Compare January 2, 2021 08:04

digisomni added this to the 2021.1.0 Release milestone Jan 2, 2021

digisomni reviewed Jan 2, 2021

View reviewed changes

HifiExperiments added 2 commits January 2, 2021 17:13

Merge remote-tracking branch 'upstream/master' into pivot

a072010

more fixes and create checkbox defaults to checked

5dc45e8

ctrlaltdavid requested changes Jan 3, 2021

View reviewed changes

libraries/render-utils/src/Model.cpp Outdated Show resolved Hide resolved

libraries/entities/src/EntityItemProperties.cpp Show resolved Hide resolved

ctrlaltdavid added the scripting api change Change is made in the scripting API label Jan 3, 2021

CR and actually send property over wire

030524c

HifiExperiments added the rebuild rebuild through the GithubActions label Jan 4, 2021

HifiExperiments added rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Jan 4, 2021

ctrlaltdavid requested changes Jan 4, 2021

View reviewed changes

libraries/entities/src/EntityItemProperties.cpp Outdated Show resolved Hide resolved

ctrlaltdavid approved these changes Jan 8, 2021

View reviewed changes

HifiExperiments added 2 commits January 21, 2021 21:05

non-exact collisions should work on models with pivots and useOrigina…

bb898aa

…lPivot=true

Merge remote-tracking branch 'origin/pivot' into pivot

7bed9b0

HifiExperiments added rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Jan 22, 2021

HifiExperiments added 2 commits February 2, 2021 22:27

Merge remote-tracking branch 'upstream/master' into pivot

56fa143

dirty physics on useOriginalPivot change, other small fix for compoun…

ace6bf6

…d shapes

add note about only supporting 0,0,0 pivot

766b91f

digisomni merged commit b826743 into vircadia:master Feb 10, 2021

HifiExperiments deleted the pivot branch February 10, 2021 06:37

HifiExperiments mentioned this pull request Feb 11, 2021

Support non-zero pivot points for models with useOriginalPivot #1018

Open

SilverfishVR mentioned this pull request Mar 17, 2021

collision breaks on animated models #1100

Closed

	DEFINE_PROPERTY_REF(PROP_USE_ORIGINAL_PIVOT, UseOriginalPivot, useOriginalPivot, bool, false);
	DEFINE_PROPERTY_REF(PROP_USE_ORIGINAL_PIVOT, UseOriginalPivot, useOriginalPivot, bool, true);

Model Entities: useOriginalPivot Property #951

Model Entities: useOriginalPivot Property #951

Uh oh!

Conversation

HifiExperiments commented Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vircadia-build-notifier commented Jan 2, 2021

Uh oh!

vircadia-build-notifier commented Jan 3, 2021

Uh oh!

Uh oh!

Uh oh!

vircadia-build-notifier commented Jan 4, 2021

Uh oh!

vircadia-build-notifier commented Jan 4, 2021

Uh oh!

vircadia-build-notifier commented Jan 4, 2021

Uh oh!

Uh oh!

Aitolda commented Jan 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HifiExperiments commented Jan 22, 2021

Uh oh!

vircadia-build-notifier commented Jan 22, 2021

Uh oh!

vircadia-build-notifier commented Jan 22, 2021

Uh oh!

vircadia-build-notifier commented Jan 23, 2021

Uh oh!

Aitolda commented Jan 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HifiExperiments commented Feb 4, 2021

Uh oh!

vircadia-build-notifier commented Feb 4, 2021

Uh oh!

Aitolda commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aitolda commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aitolda commented Feb 5, 2021

Uh oh!

HifiExperiments commented Feb 6, 2021

Uh oh!

digisomni commented Feb 6, 2021

Uh oh!

vircadia-build-notifier commented Feb 6, 2021

Uh oh!

AleziaKurdis commented Feb 10, 2021

Uh oh!

HifiExperiments commented Feb 10, 2021

Uh oh!

Uh oh!

HifiExperiments commented Jan 2, 2021 •

edited

Loading

Aitolda commented Jan 9, 2021 •

edited

Loading

Aitolda commented Jan 23, 2021 •

edited

Loading

Aitolda commented Feb 4, 2021 •

edited

Loading

Aitolda commented Feb 4, 2021 •

edited

Loading