Entity update improvements 2 by HifiExperiments · Pull Request #830 · vircadia/vircadia-native-core

HifiExperiments · 2020-10-21T00:16:41Z

also fixes #681

The current rendering code is a mess of locks and threads and chaos. This is an attempt to disentangle some things, hopefully resulting in some performance improvements.

The general idea is:

EntityItems must be threadsafe as they are accessed by multiple threads (main thread, script threads, render thread)
RenderableEntityItems operate on two threads, the main thread (doRenderUpdateSynchronous, needsRenderUpdate) and the render thread (doRenderUpdateAsynchronous, doRender)
RenderableEntityItems should do as much work as possible in doRenderUpdateAsynchronous so that we don't have to lock as much.
if a property is accessed on both the main thread and the render threads, it needs to be locked properly
needsRenderUpdate is ONLY called when an EntityItem is edited. if a Renderable needs to update constantly to check for something (like textures loading or model animation), it just needs to emit requestRenderUpdate() and needsRenderUpdate can be removed

Test plan:

To test the model dimensions/collision bug fix, run this script: https://gist.githubusercontent.com/HifiExperiments/5b3a0207e222546b27ed182244a6861a/raw/cce164f967f7632cdccc845b75028e64ee1ef4ce/ModelTest.js
Press o to toggle the modelURL. Verify that the dimensions and the collision shape are correct.
Confirm the test plan from Shape Entity: Broken display when alpha is set. #850 works
For the rest of the PR, confirm that modifying various properties that effect rendering still work. In particular, for every entity type (but especially models), confirm that the following still work (as they do in master, some entities don't support these properties): visibility, shadows, render layer, primitive mode, renderWithZones, position/orientation/scale
confirm that models parented to your head joint properly cauterize (don't render) when you're in first person, but do render normally otherwise

vircadia-build-notifier · 2020-10-21T02:25:02Z

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

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

vircadia-build-notifier · 2020-10-21T04:22:02Z

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/PR830/Vircadia-Alpha-PR830-392a344_115229da.dmg

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR830/Vircadia-Alpha-PR830-392a344_115229da.exe

ctrlaltdavid · 2020-10-21T08:28:22Z

Initial testing ...

Clear cache (Developer > Network > Clear Disk Cache).
Restart Interface.
Enable display of Bullet collision hulls (Developer > Physics > Show Bullet Collisions).
Run script...

Two green cubes glTF model displayed initially, with expected collision hulls.
Press "o".
Model changed but collision hull not updated. (Collision hull for initial two cubes still displayed and you can walk through the single cube FBX model.)

Restart Interface, and the correct collision hull is displayed for the FBX model.
Run the script again.
This time, when the two (new) green cubes changes to the single cube FBX, the correct collision hull is displayed and you cannot walk through the model.

Delete the models and restart Interface.'
Run the script again.
Same problem occurs.

Note: Problem seems not to occur when updating the model using the Create app.

vircadia-build-notifier · 2020-10-22T00:39:43Z

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/PR830/Vircadia-Alpha-PR830-676ca8e_f413ad33.dmg

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR830/Vircadia-Alpha-PR830-676ca8e_f413ad33.exe

vircadia-build-notifier · 2020-11-05T05:22:03Z

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

build (windows-latest, full)

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

build (macOS-latest, full)

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

vircadia-build-notifier · 2020-11-05T08:12:28Z

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/PR830/Vircadia-Alpha-PR830-4634244_c8532a1a.dmg

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR830/Vircadia-Alpha-PR830-4634244_c8532a1a.exe

Co-authored-by: David Rowe <[email protected]>

HifiExperiments · 2020-11-27T23:12:42Z

@SilverfishVR thanks for catching that! I think it should be fixed now

vircadia-build-notifier · 2020-11-28T01:02:36Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR830/Vircadia-Alpha-PR830-705a90f_9a5b9569.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR830/Vircadia-Alpha-PR830-705a90f_9a5b9569.exe

build (ubuntu-18.04, android)

SilverfishVR · 2020-11-28T18:32:25Z

PR830-705a90f:

missing collision hull seems fixed now 👍 However, reloading content disables it long enough for avatars to fall through, So if you stand on the default sandbox builder grid, and reload content, you fall through to land on the "Grid-Collider" box entity below, then the "builders_grid_bottom" collider (good - sub-meshes) comes back and traps you inside.
it may not be a big issue but we would have to modify the default sandbox content to avoid this.
In general I think we need to look at how avatar collision is handled during content load anyway.
Shape Entity: Broken display when alpha is set. #850 seems to work as expected 👍
visibility, shadows, render layer, primitive mode, renderWithZones, materialEntities, Custom Shader on entity, blendshapes and occlusion when parented to head all seems OK 👍
tl:dr appart from less than ideal behaviour during content load all seems to work

ctrlaltdavid · 2020-12-03T22:23:42Z

Need to test both connecting to a domain and reloading content, in current 2020.3.2 release and this PR.

vircadia-build-notifier · 2020-12-04T00:13:48Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR830/Vircadia-Alpha-PR830-5e5e507_9a5b9569.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR830/Vircadia-Alpha-PR830-5e5e507_9a5b9569.exe

build (ubuntu-18.04, android)

SilverfishVR · 2020-12-04T22:27:25Z

did a fresh install with PR830-5e5e507 to test the collision hull loading issue:

I can't trigger it on domain change, only on reload.
It only happens if I have more than the entity I'm landing on set to "good" maybe it is still busy generating hulls for the other entities when I land, so it will sort of random if the collision is ready in time when I land 🤔
I don't get it on 2020.3.2, although, falling through the ground is still a thing sometimes Users sometimes stuck underground #757 so maybe time is better spend ensuring that avatar "drop" is good in all cases, that would be a seperate PR. the default sandbox can be easily fixed by moving the box collider that is already there up a bit, I think it is there because hifi knew that sometimes, just sometimes you fall 😄

SilverfishVR · 2020-12-05T00:50:19Z

so, there is definitely some differences in how model scale is handled with this pr:

in 2020.3.2
Load this: https://silverfish-freestuff.s3.eu-north-1.amazonaws.com/TestStuff/ModelDimensionTest.js
you should see a 1m radius sphere (2x2m bounding box).
press o to change it to a 2x2x8m rectangle, that is correct but of course the collision hull is still a sphere and the bounding box, as shown by selecting it in create mode is still 2x2x2m

so, model size is correct but bounding box and collision is off. And if you rescale the rectangle in create, and switch to the sphere, it will be squished because the aspect ratio is off.

now in PR830-5e5e507 do the same.
the rectangle will be squished into the 2x2x2m bounding box of the original model, if you reset dimensions, it goes back to the correct aspect but, well, going back to the sphere it will be elongated to 2x2x8m, but the bounding box and collision always match the model.

So, I think there is no "solve it all" solution to this, since only size(x,y,z) is stored, but not scale(x,y,z), wearables used to apply some form of arbitrary scale and that f'ed up all the time so I would not want to apply that kind of complex logic to models in general. The behaviour of this PR of keeping collision and bounding box in line with the displayed model is prefered, but maybe the scale should be reset when changing the model url? it will also be very annoying when iterating through models that are not to scale, having to rescale every time 🤔 pick your poison I guess 😃

HifiExperiments · 2020-12-05T04:12:43Z

@SilverfishVR that scaling behavior is intentional, and I think it’s in line with what was originally intended by the code. the problem is that if someone manually modified the dimensions to something other than the natural dimensions, then I’d expect them to want those dimensions to carry over when the URL changes. but I agree it might not always be what they want otherwise. but I think this way is most flexible. people can always set the dimensions to the new naturalDimensions, but if we did the other behavior, there would be no way to “go back” to the OLD dimensions.

regarding reloading content, it sounds like the problem is that we need safe landing to start again, but since safe landing doesn’t work perfectly, that might not be ideal. reloading content is supposed to be more of a development tool so I think this is ok for now

digisomni

With some cursory testing over all the test cases, everything appears to be working as expected. :)

vircadia-build-notifier · 2020-12-12T20:34:33Z

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

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR830/Vircadia-Alpha-PR830-4d7d026_9a5b9569.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR830/Vircadia-Alpha-PR830-4d7d026_9a5b9569.exe

HifiExperiments added 6 commits October 12, 2020 18:50

starting improve entity updates pt 2

5a8c1a0

more updates

ae7a46f

collisions and dimensions seem to be working

9142b71

what about...this

44b34e0

models seem to be working!

c664991

more improvements

2dd612b

HifiExperiments added enhancement New feature or request renderer Touches or changes rendering needs testing (QA) The PR is ready for testing bugfix high risk threading/deadlock (beware) needs CR (code review) labels Oct 21, 2020

two-one-five added this to the 2020.3.2 Release milestone Oct 21, 2020

two-one-five requested a review from ctrlaltdavid October 21, 2020 00:43

small fixes

081e2b1

build error

115229d

small fixes

f413ad3

HifiExperiments force-pushed the update branch from 56673f0 to f413ad3 Compare October 21, 2020 23:06

two-one-five requested a review from daleglass October 29, 2020 21:46

possible fix for collisions again

a3c85bd

fix alpha material bug

a7e6332

HifiExperiments linked an issue Nov 5, 2020 that may be closed by this pull request

Shape Entity: Broken display when alpha is set. #850

Closed

another unused variable

c8532a1

HifiExperiments and others added 2 commits November 27, 2020 15:10

add comment about material sphere scale

c510a8f

Apply suggestions from code review

9a5b956

Co-authored-by: David Rowe <[email protected]>

ctrlaltdavid approved these changes Nov 28, 2020

View reviewed changes

two-one-five added CR Approved At least one code reviewer has approved the PR. rebuild rebuild through the GithubActions and removed needs CR (code review) labels Dec 3, 2020

two-one-five added rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Dec 10, 2020

HifiExperiments added rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Dec 11, 2020

two-one-five added rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Dec 12, 2020

digisomni mentioned this pull request Dec 12, 2020

Shape Entity: Broken display when alpha is set. #850

Closed

digisomni approved these changes Dec 12, 2020

View reviewed changes

digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Dec 14, 2020

digisomni merged commit 8c4ce96 into vircadia:master Dec 14, 2020

HifiExperiments deleted the update branch December 14, 2020 23:30

HifiExperiments mentioned this pull request Dec 17, 2020

Changing the modelURL (to a new model) of an already placed model causes the physics to not update #189

Closed

digisomni removed the request for review from daleglass December 28, 2020 05:34

HifiExperiments mentioned this pull request Dec 29, 2020

Possible fix for bad physics performance #944

Merged

Comments

Conversation

HifiExperiments commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vircadia-build-notifier commented Oct 21, 2020

Uh oh!

vircadia-build-notifier commented Oct 21, 2020

Uh oh!

ctrlaltdavid commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vircadia-build-notifier commented Oct 22, 2020

Uh oh!

vircadia-build-notifier commented Nov 5, 2020

Uh oh!

vircadia-build-notifier commented Nov 5, 2020

Uh oh!

HifiExperiments commented Nov 27, 2020

Uh oh!

vircadia-build-notifier commented Nov 28, 2020

Uh oh!

SilverfishVR commented Nov 28, 2020

Uh oh!

ctrlaltdavid commented Dec 3, 2020

Uh oh!

vircadia-build-notifier commented Dec 4, 2020

Uh oh!

SilverfishVR commented Dec 4, 2020

Uh oh!

SilverfishVR commented Dec 5, 2020

Uh oh!

HifiExperiments commented Dec 5, 2020

Uh oh!

digisomni left a comment

Choose a reason for hiding this comment

Uh oh!

vircadia-build-notifier commented Dec 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HifiExperiments commented Oct 21, 2020 •

edited

Loading

ctrlaltdavid commented Oct 21, 2020 •

edited

Loading