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

Configurable Camera Raycast + Avatar Upload Fix #5881

Merged
merged 33 commits into from
May 7, 2022
Merged

Conversation

dinomut1
Copy link
Member

@dinomut1 dinomut1 commented Apr 28, 2022

Summary

Camera raycasting is one of the most expensive systems in the engine. This PR adds config settings for the camera raycasting that can be accessed in a scene's root node in editor. Camera raycasting is set to be disabled by default.

Also fixes an error with avatar uploading by checking for AvatarAnimation and Velocity components before performing bone retargeting.

Checklist

  • CI/CD checks pass npm run check
    • Linter passing via npm run lint
    • Typescript passing via npm run check-errors
    • Unit & Integration tests passing via npm run test
    • Docker build process passing via npm run build-client
  • If this PR is still a WIP, convert to a draft
  • When this PR is ready, mark it as "Ready for review"
  • Changes have been manually QA'd
  • Changes reviewed by at least 2 approved reviewers

Reviewers

Reviewers for this PR

Copy link
Member

@HexaField HexaField left a comment

Choose a reason for hiding this comment

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

i fixed up a few convention inconsistencies, and fixed the LoopAnimationComponent error in a previous PR, so i reverted the change to avatarFunctions.ts

although i am a bit confused, why are creating a new camera serializer/deserializer rather than extending camera-properties ?

also, why is this new camera in the camera folder rather than with all the other scene loader functions in /scene/functions/loaders?

lets try and unify these to the existing camera-properties

when this is done, lets also ensure not just the default project but all the scenes have this change

we also need to be adding tests for these sorts of changes, to prove functionality (i have a few concerns about how it currently is handled either way, which should be fixed by adhering to the existing camera-properties) as well as to stop regressions

@speigg
Copy link
Member

speigg commented Apr 28, 2022

I dug into this, and what's actually slowing things down seems to not be the raycasting itself, but the fact that we have hundreds of collider objects in some scenes (270 in the globe theater). I think we need to address the root cause here. First of all, it's redundant to be checking both scene geometry and scene colliders during raycasting. Second, all these colliders are also slowing down the rendering system, since they are being iterated there every frame as well. BVH is fast, but iterating through hundreds of objects is slow.

@dinomut1
Copy link
Member Author

Merged in the camera raycasting data into the camera properties component, so that was straightforward. However I am getting the null ref regarding the AvatarAnimationComponent when doing avatar upload. If that is tied into other changes being done I won't touch it.

@dinomut1 dinomut1 requested a review from HexaField April 29, 2022 19:52
@dinomut1 dinomut1 merged commit 0662f67 into dev May 7, 2022
@dinomut1 dinomut1 deleted the raycast-config branch May 7, 2022 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants