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

BUGZ-902: Drastically reduce locking and branching necessary for render updates #16015

Merged
merged 3 commits into from Aug 15, 2019

Conversation

@HifiExperiments
Copy link
Contributor

commented Jul 31, 2019

https://highfidelity.atlassian.net/browse/BUGZ-902 + more

In the current system, needsRenderUpdateFromTypedEntity (which is called frequently, especially on dynamic objects and objects that are frequently edited) for almost all entity types has many calls to XXXEntityItem methods that lock the entire EntityItems, even in cases when none of the properties changed.

Instead: when we change a property that affects rendering, mark the EntityItem as _needsRenderUpdate. Now we just need to check that one bool, instead of doing multiple, possibly expensive equality checks every frame!

Test plan:

  • full nitpick pass
@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Can someone from High Fidelity approve this PR?

1 similar comment
@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Can someone from High Fidelity approve this PR?

interface/src/octree/SafeLanding.cpp Show resolved Hide resolved
});

return needsUpdate;
}

This comment has been minimized.

Copy link
@HifiExperiments

HifiExperiments Jul 31, 2019

Author Contributor

Most of the changes in this PR look like this: remove/simplify needsRenderUpdateFromTypedEntity, and instead set _needsRenderUpdate in XXXEntityItem::setXXX

}
if (glm::isnan(result.spin.range.finish)) {
result.spin.range.finish = getParticleSpin();
}

This comment has been minimized.

Copy link
@AndrewMeadows

AndrewMeadows Aug 1, 2019

Contributor

Hrm... I see. We check everything for nan here when we get the properties out. So the values in the ParticleEffectEntityItem can be nan but as per this code... we won't supply a particle::Properties with nan.

@HifiExperiments HifiExperiments changed the title Drastically reduce locking and branching necessary for render updates BUGZ-902: Drastically reduce locking and branching necessary for render updates Aug 1, 2019

@HifiExperiments HifiExperiments changed the title BUGZ-902: Drastically reduce locking and branching necessary for render updates DNM: BUGZ-902: Drastically reduce locking and branching necessary for render updates Aug 1, 2019

@ZappoMan
Copy link
Contributor

left a comment

Project approval.

@ZappoMan ZappoMan added this to the v0.84.0 milestone Aug 1, 2019

@MiladNazeri

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

add to whitelist

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Android build is available here. Quest build is available here

@shanzzam

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@HifiExperiments this is looking good but has conflicts. Can you please resolve and then we can approve and merge?

@HifiExperiments HifiExperiments changed the title DNM: BUGZ-902: Drastically reduce locking and branching necessary for render updates BUGZ-902: Drastically reduce locking and branching necessary for render updates Aug 5, 2019

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Android build is available here. Quest build is available here

@HifiExperiments

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@shanzzam @ZappoMan @AndrewMeadows this is ready to go

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Android build is available here. Quest build is available here

@ZappoMan
Copy link
Contributor

left a comment

re-PA

@ZappoMan ZappoMan merged commit b0b406a into highfidelity:master Aug 15, 2019

2 checks passed

default Build finished.
Details
license/cla Contributor License Agreement is signed.
Details

@HifiExperiments HifiExperiments deleted the HifiExperiments:renderUpdate branch Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.