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

ADR 256: static entities #281

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

ADR 256: static entities #281

wants to merge 3 commits into from

Conversation

nearnshaw
Copy link
Member

No description provided.

@nearnshaw nearnshaw requested a review from a team as a code owner January 3, 2025 19:56
@nearnshaw nearnshaw requested a review from aleortega January 3, 2025 19:56
Copy link

cloudflare-workers-and-pages bot commented Jan 3, 2025

Deploying adr with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3273939
Status: ✅  Deploy successful!
Preview URL: https://13b23549.adr-cvq.pages.dev
Branch Preview URL: https://static-entities.adr-cvq.pages.dev

View logs

Note: As an alternative approach, we also discussed handling this via a separate new `Static` component instead of a new field on the `Transform` component. There are pros and cons to each approach.

- On one hand, an extra field on the Transform adds a slight overhead to each update message regarding any transform. (Although if the value of its corresponding protobuff digit is 1, then there should be no Transform updates beyond the initial creation)
- On the other hand, it's easier to creators to find where to add this property if it exists on the Transform. Any object that makes sense to make static will always have a Transform anyway.

Choose a reason for hiding this comment

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

I'd choose Static component rather than an extra bool in Transform.

  1. Static feature affect many other components beside Transform, it makes more sense to implement it as a new component in my opinion
  2. I'd say that it would affect L-caching performance on engine's side because each extra field makes an array of structures bigger and it turns to handle less components at the time. But this aspect could be easily optimised

@kuruk-mm
Copy link
Member

kuruk-mm commented Jan 10, 2025

Adding a field to the Transform breaks the compatibility of the protocol. The Transform is not a protobuf; it's a fixed set of bytes. Adding a new field changes its length, which makes it problematic. Additionally, it's the most used component. Adding one byte that isn't going to be used is not a good option.

Optionally, we can use a new StaticComponent as @NickKhalow said. To make things easier for Creators, we could create a wrapper in the SDK that behaves like a new field in the Transform but adds the StaticComponent behind the scenes.

Almost every time we make a protocol change to "make things easier for users," the solution doesn’t lie in the protocol itself—it lies in the SDK implementation. The protocol should remain as simple as possible, serving as an efficient communication channel between the runtime and the explorers.

Anyway, we have another proposal for this approach that we think is better for the Creator and also easier for the Protocol: adding this function to the engine API.

Example code:

import { markEntitiesAsStatic } from '~system/EngineApi'

export function main() {
  const door = engine.addEntity()

  GltfContainer.create(door)
  markEntitiesAsStatic([door])

  GltfContainer.update(door) // it should fail at the SDK level... at the engine level, it does nothing
}

WDYT?

Also. It would be interesting to know which kind of optimization the Foundation is thinking.

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.

3 participants