feat: sdk7 GltfContainerLoadingState component
#5063
Conversation
| @@ -11,6 +11,7 @@ public class GltfContainerLoadingStateSystem | |||
| { | |||
| private readonly IInternalECSComponent<InternalGltfContainerLoadingState> gltfContainerLoadingStateComponent; | |||
| private readonly IECSComponentWriter componentWriter; | |||
| private int timestamp = 0; | |||
There was a problem hiding this comment.
This timestamp should be entity aware. I think you are setting it globally, is that correct?
There was a problem hiding this comment.
that was because I wasn't storing it at the crdt state, but I'm changing it right now, so timestamp will be at entity level, being prevComponentStateForEntity.timestamp +1
AjimenezDCL
requested review from
mikhail-dcl
and removed request for
AjimenezDCL
|
|
||
| public void Update() | ||
| { | ||
| var components = gltfContainerLoadingStateComponent.GetForAll(); |
There was a problem hiding this comment.
Currently this implementation traverses all the entities with IInternalECSComponent<InternalGltfContainerLoadingState>, so basically EVERY entity with a GLTF (since we are adding that internal component to all).
Wouldn't it be better to just traverse the entities that have the new (not internal) GltfContainerLoadingState component instead? since those would be the only ones that we are interested in updating and reporting its loading state...
Maybe I'm missing the point here as I thought that to use the newGltfContainerLoadingState component from a scene with the SDK it should be added from the scene to an entity and then read it later... if we are adding that data to EVERY entity with GltfContainer why not add and update that loading state property in the GltfContainer component itself??? and avoid having a new unneeded component.
There was a problem hiding this comment.
Wouldn't it be better to just traverse the entities that have the new (not internal) GltfContainerLoadingState component instead?
No, since scene won't be sending the GltfContainerLoadingState to the renderer, the renderer will be sending the GltfContainerLoadingState to the scene. It's not sent by request, for every GltfContainer we expect to receive a GltfContainerLoadingState
Maybe I'm missing the point here as I thought that to use the newGltfContainerLoadingState component from a scene with the SDK it should be added from the scene to an entity and then read it later... if we are adding that data to EVERY entity with GltfContainer why not add and update that loading state property in the GltfContainer component itself??? and avoid having a new unneeded component.
GltfContainer is sent from the scene to the renderer, not the other way around
GltfContainerLoadingState is sent from the renderer to the scene, not the other way around
so we need two different components to avoid any write conflict.
plus, they are related but conceptually different components, one contains data of the gltf and the other the data of it loading state
There was a problem hiding this comment.
Ok, that explains why we couldn’t have that property in the GltfContainer itself, but why would we put the GltfContainerLoadingState on every entity that has the GltfContainer instead of leaving the usage of the GltfContainerLoadingState to the creators through the SDK in their scene code? that way Creators may only add that GltfContainerLoadingState component to entities that they really will be reading.
The current proposed implementation forces the client to traverse every entity that has a GltfContainer component (since we put the GltfContainerLoadingState on every entity that has a GltfContainer) when maybe none of them is actually read by the scene’s code in the end...
Also if the GltfContainerLoadingState is only sent by the renderer, it should probably be just a GrowOnlyValueSet component (using Append to update it from the renderer) instead of a LastWriteWin one, since the scene will never change it, just read it. But that can be changed in the future when decentraland/sdk#739 is tackled
What does this PR change?
add
GltfContainerLoadingStatecomponentfixes decentraland/sdk#736
adr https://adr.decentraland.org/adr/ADR-215
*most of the files modifications are related to a type changed on the protocol
Our Code Review Standards
https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md
Copilot summary
🤖 Generated by Copilot at 4e9a408
This pull request adds a new internal component
InternalGltfContainerLoadingStateto the ECS7 plugin, which tracks the loading and removal ofGltfContainercomponents for each entity. It also updates the existing ECS7 components and systems to use this new component and changes the type of some collider fields frominttouintfor consistency and performance. It modifies the assembly definitions and meta files accordingly.