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

Move InternalJobIndex to PartitionComponent #2895

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

AnsisMalins
Copy link
Collaborator

@AnsisMalins AnsisMalins commented Nov 26, 2024

What does this PR change?

This change moves the InternalJobIndex property from ScenePartitionComponent to PartitionComponent. Reasons:

  • ScenePartitionComponent is a value type, so every new instance is new, but PartitionComponent is a pooled reference type. Without this change, a new InternalJobIndex would be assigned to each new ScenePartitionComponent until we run out (the partition data buffer is preallocated to 90000 items). With this change, the job indices and partition datas are reused because the partition component instances are reused.
  • InternalJobIndex points to a PartitionData. I believe that PartitionData better belongs to PartitionComponent and not SceneDefinitionComponent.

Do note that this is just the second in a series of many pull requests to refactor and optimize the partition components. The first one was #2833

How to test the changes?

  1. Launch the explorer
  2. Play normally
  3. The game should work exactly the same as without this change

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

@AnsisMalins AnsisMalins force-pushed the partition-component-internaljobindex branch from 3a5e333 to a07e5e5 Compare November 28, 2024 08:26
@AnsisMalins AnsisMalins marked this pull request as draft December 9, 2024 17:07
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.

2 participants