Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Fix component reactivity #7615

Merged
merged 7 commits into from
Feb 24, 2023
Merged

Fix component reactivity #7615

merged 7 commits into from
Feb 24, 2023

Conversation

speigg
Copy link
Member

@speigg speigg commented Feb 20, 2023

Summary

This PR addresses superfluous state invalidations that were causing some reactors to re-run unnecessarily, including it's side-effects (after making the drop-shadow reactor reactive to changes in the GroupComponent to address the issue described in the note below, these superfluous state invalidations became evident).

More details on this hookstate issue here: avkonst/hookstate#299

The solution involves placing all component instances in their own hookstate proxy, and reactively keeping track of component additions/removals in a stateful Component.existenceMap. With each component instance isolated in this way, changes to component data should only affect the reactors that are actually using that data.

Note:
This PR also cleans up some issues w/ the drop-shadow reactor, e.g., a race condition where it would generate a shadow too small for an avatar before the avatar had a chance to load it's model.

@speigg
Copy link
Member Author

speigg commented Feb 21, 2023

gotta fix a failing test

Copy link
Member

@hanzlamateen hanzlamateen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Not sure if it is related, but I get following hookstate error when I change a scene from Scene from Apartment to Sky Station.
    image

  2. Similarly when I click on 'Portal - Sky Station Interior' & then 'Portal - Sky Station Exterior' in heirarchy panel then it fails due to hookstate error.
    image
    image

@speigg
Copy link
Member Author

speigg commented Feb 23, 2023

@hanzlamateen thank you that's very helpful

@AidanCaruso
Copy link
Member

AidanCaruso commented Feb 23, 2023

In an apartment scene in which I have placed 6 cyberbots I've noticed low framerates after loading the models in, this was not an issue on dev, and I switched between the two to check, going to triple check now because I'm not immediately sure why this would be happening.

Follow up: Switching to dev I got expected framerates, but after switching back to this branch this issue returns.

image
image

@AidanCaruso
Copy link
Member

AidanCaruso commented Feb 23, 2023

Also getting errors similar to Hanzla when duplicating objects. Or dragging in more than one cyberbot model. This was working when I tried earlier today on dev before checking out my own branch. Dragging in one of those models also brings down framerate.
image

@speigg
Copy link
Member Author

speigg commented Feb 23, 2023

Low framerates when models are dropped in seems related to raycasts for drop shadows. We need to enable bounding volume hierarchies by default to speed up CPU raycasts. Currently the default is to have them disabled. I'll invert that.

@HexaField
Copy link
Member

Low framerates when models are dropped in seems related to raycasts for drop shadows. We need to enable bounding volume hierarchies by default to speed up CPU raycasts. Currently the default is to have them disabled. I'll invert that.

We also should not do any drop shadows logic in the editor

@speigg
Copy link
Member Author

speigg commented Feb 23, 2023

Ready to merge this in;

In separate PRs:

  • Will update default generateBVH property to true, and update our scenes (this resolves perf issues w/ drop shadows)
  • Visualize BVH, expose more settings
  • Will update useComponent to suspend/resume react components (this will take care of some of the errors we see in the console)

@AidanCaruso
Copy link
Member

Low framerates when models are dropped in seems related to raycasts for drop shadows. We need to enable bounding volume hierarchies by default to speed up CPU raycasts. Currently the default is to have them disabled. I'll invert that.

We also should not do any drop shadows logic in the editor

This is actually a super good point.

@speigg speigg added this pull request to the merge queue Feb 24, 2023
Merged via the queue into dev with commit ef0cb2a Feb 24, 2023
@speigg speigg deleted the fix-component-state-issues branch February 24, 2023 03:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants