Feat/asset packs validation - #1263
Conversation
Test @dcl/asset-packs package
|
Test @dcl/inspector package
|
Test this pull request on macos-latestDownload the correct version for your architecture:Click here if you don't know which version to downloadFor running this unsigned version of the app, you will need to run the xattr command on it:
|
Test this pull request on windows-latestDownload the correct version for your architecture: |
alejandralevy
left a comment
alejandralevy
left a comment
There was a problem hiding this comment.
🔍 Code Review
Nice validation system! The architecture is clean — rules are well-isolated, helpers are reusable, and the CI integration is solid. A few things I noticed:
Issues
1. ✅ printed even when asset has errors (validate.ts)
After printing errors for an asset, the code unconditionally logs asset.name, '✅'. This is misleading in CI logs — someone skimming would think everything passed. Suggest:
if (errors.length === 0) console.log(asset.name, '✅');2. hasPointerCollider may be too permissive (helpers.ts)
It returns true for any GltfContainer regardless of collision masks. But the SDK defaults defined in the same file show GLTF_DEFAULT_VISIBLE_MASK = 0 and GLTF_DEFAULT_INVISIBLE_MASK = CL_PHYSICS — neither includes CL_POINTER. So an asset with a GltfContainer but no pointer collision mask would pass this check incorrectly.
If GltfContainer visible meshes are implicitly raycastable by the engine (regardless of masks), this is fine and worth a comment. Otherwise, consider checking the actual masks for the CL_POINTER bit like hasAnyCollisionMask does for its checks.
3. Collision mask compared with > 0 instead of !== 0 (helpers.ts)
Minor: meshCollider.collisionMask > 0 and visibleMask > 0 — for bitmask checks, !== 0 is idiomatic and safer (handles theoretical cases where high bits could produce negative values in 32-bit signed interpretation). Not a real risk with small DCL masks, but a good habit.
Suggestions
4. No tests for the validation rules
This is a CI-blocking validation system with 9 rules and non-trivial logic (bitwise ops, cross-component checks, self-reference detection). Unit tests for each rule would be straightforward since validateComposite takes plain data — no mocks needed. Particularly worth testing:
- Edge cases in
hasPointerCollider/hasAnyCollisionMaskwith various mask values trigger-action-references-must-resolvewith different{self}patternsinvisible-collider-needs-collision-maskskip logic (set_visibility, triggers/actions)- Empty/missing component arrays
5. Rule numbering gap (rules.ts)
Rules jump from 3 → 5 → 6 → 7 → 10 → 11 → 12 → 13. Looks like leftover numbering from a design doc. Consider renumbering for clarity, or add a comment referencing the design doc.
What looks good ✅
- Clean separation: helpers → rules → index → validate script
- Smart skip logic in
invisible-collider-needs-collision-mask(checking forset_visibilityactions across all entities) text-shape-mutually-exclusivecorrectly checks per-entity key overlap instead of just component presence- Error vs warning severity split is well-thought-out
- The
flatMappattern invalidateCompositeis elegant
| for (const error of errors) { | ||
| const prefix = error.severity === 'error' ? '❌' : '⚠️'; | ||
| console.error(`${prefix} [${error.rule}] ${error.message}`); | ||
| if (error.severity === 'error') allErrors.push(error); |
There was a problem hiding this comment.
This console.log(asset.name, '✅') runs even when the asset has validation errors — misleading in CI logs. Wrap it:
if (errors.length === 0) console.log(asset.name, '✅');| // GltfContainer models support pointer raycasting via their visible meshes | ||
| // and internal *_collider meshes, even without explicit collision masks | ||
| if (hasComponent(components, 'core::GltfContainer')) return true; | ||
|
|
There was a problem hiding this comment.
This returns true for any GltfContainer regardless of masks. But the defaults above show GLTF_DEFAULT_VISIBLE_MASK = 0 (no pointer) and GLTF_DEFAULT_INVISIBLE_MASK = CL_PHYSICS (no pointer). Is this intentional because the engine always raycasts visible GLTF meshes? If so, a comment would help. Otherwise, check the masks for CL_POINTER bit like:
const gltf = getComponentData(components, 'core::GltfContainer');
if (gltf) {
const vis = gltf.visibleMeshesCollisionMask ?? GLTF_DEFAULT_VISIBLE_MASK;
const invis = gltf.invisibleMeshesCollisionMask ?? GLTF_DEFAULT_INVISIBLE_MASK;
if ((vis & CL_POINTER) !== 0 || (invis & CL_POINTER) !== 0) return true;
}| */ | ||
| export function hasAnyCollisionMask(components: Component[]): boolean { | ||
| const meshCollider = getComponentData(components, 'core::MeshCollider'); | ||
| if (meshCollider && meshCollider.collisionMask > 0) return true; |
There was a problem hiding this comment.
Nit: meshCollider.collisionMask > 0 — for bitmask checks, !== 0 is more idiomatic and technically safer. Same for visibleMask > 0 || invisibleMask > 0 below.
1 participant
Improve Smart items validations — CI validation rules for asset packs
Adds a composite validation system to the asset-packs CI pipeline that checks component dependencies in smart item
composite.jsonfiles. This catches broken items before they reach production.What it does
Expands
make validate-asset-packs(already runs in CI on every asset-packs PR) with 9 validation rules that check component relationships across all 214 smart items.Validation rules
Errors (block CI):
pointer-events-requires-collider🎯 — If an entity hasPointerEventsor anon_input_action/on_clicktrigger, it must have a collider (MeshCollider or GltfContainer)invisible-collider-needs-collision-mask👻 — If an entity hasVisibilityComponent(visible: false)+GltfContainer, at least one collision mask must be > 0 (otherwise it has no effect at runtime). Skips items that useset_visibilityto toggle at runtimetrigger-action-references-must-resolve🔫 — Self-referencing trigger actions must point to an action name that actually exists in the entity's Actions componentanimator-requires-gltf👾 ** — If an entity hasAnimator, it must haveGltfContainer(animations live inside GLTF models)video-player-requires-display🎥 — If an entity hasVideoPlayer, it must have a texture source (GltfNodeModifiersorMaterial) and a render surface (GltfContainerorMeshRenderer)trigger-conditions-reference-valid-components🔫 — Trigger conditions referencing states (when_state_is) or counters (when_counter_equals) must have the correspondingStatesorCountercomponent on the entityWarnings (don't block CI):
states-must-be-referenced😄 — If an entity definesStates, they should be referenced somewhere in triggers or actionsactions-unique-names📛 — Action names within anActionscomponent should be uniquetext-shape-mutually-exclusive✍️: —TextShapeshould not coexist withMeshRendererorGltfContaineron the same entity (they are mutually exclusive per SDK spec). Multi-entity composites where TextShape is on a child entity are fine