APP-14530 3 add snapshot utils #322
Conversation
…add-snapshot-utils
…onent_add-snapshot-utils
…add-snapshot-utils
|
src/lib/snapshot.ts
Outdated
| } | ||
| } | ||
|
|
||
| const rgbaToHex = (rgba: Uint8Array): string => { |
There was a problem hiding this comment.
opt: some of these functions seem more common than just for snapshot (e.x. rgbaToHex, destroyEntities, getRenderArmModels, maybe we could split them out into separate util ts (can do as follow up)
There was a problem hiding this comment.
Yeah I could see them being used elsewhere. Happy to split them out into separate units now or we can hold off when we want to use them elsewhere. I'm good with whatever folks prefer.
src/lib/snapshot.ts
Outdated
| } | ||
| } | ||
|
|
||
| const rgbaToHex = (rgba: Uint8Array): string => { |
There was a problem hiding this comment.
nit rgbToHex? since you don't use the alpha
src/lib/snapshot.ts
Outdated
| for (const entity of subEntities) { | ||
| entities.push(world.spawn(...entity)) | ||
| } | ||
| return entities |
There was a problem hiding this comment.
shouldn't we also spawn the parent entity and add return it in the if part of this clause too? so it returns the sub entities AND the entityTraits
There was a problem hiding this comment.
Good question, I actually had that originally then went to this. The issue is, the parent entity doesn't really have anything to render, since really what we are rendering are its individual assets. So I guess the question is, do we care to add a non-rendering entity that we tie to all of the sub entities, or do we just care about the rendering sub-entities.
I am not 100% sure, so definitely would like feedback on this.
There was a problem hiding this comment.
I think we should, it still has a pose and parent so we should render it as an axis like we do for the arm frames
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| const entities = spawnSnapshotEntities(world, snapshot) | ||
|
|
||
| expect(entities).toHaveLength(1) | ||
| expect(world.query()).toHaveLength(1) |
There was a problem hiding this comment.
Simplified these tests and used koota itself instead of mocks. Since koota is mostly imperative it's very straightforward to make predictable tests that peek into what we want to know without having to mock it
PR Stack: