Skip to content

Fix: Support custom asset loading for GSplat (getter-only props) #316

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.

Sign up for GitHub

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

Open
No-LAN-B wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix: Support custom asset loading for GSplat (getter-only props) #316

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d54ff0a
Read Only Implementation For Getter-Only Properties (Schema Generator)
Feb 27, 2026
087ddbc
Corrected Logic For Blocks Like Mat4 That Do Not Have An Apply Block
Feb 27, 2026
d80fca7
reverting pnpm-lock.yaml to prefork state
Feb 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 76 additions & 28 deletions packages/lib/src/utils/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export type PropValidator<T, InstanceType> = {
errorMsg: (value: unknown) => string;
default: T | unknown;
apply?: (container: InstanceType, props: Record<string, unknown>, key: string) => void;
readOnly?: boolean;
}

// A more generic schema type that works with both functions
Expand Down Expand Up @@ -219,37 +220,36 @@ export function validatePropsWithDefaults<T extends object, InstanceType>(
* @param props The props to apply
*/
export function applyProps<T extends Record<string, unknown>, InstanceType>(
instance: InstanceType,
schema: Schema<T, InstanceType>,
instance: InstanceType,
schema: Schema<T, InstanceType>,
props: T
) {
Object.entries(props as Record<keyof T, unknown>).forEach(([key, value]) => {
if (key in schema) {
const propDef = schema[key as keyof T] as PropValidator<T[keyof T], InstanceType>;
if (propDef) {
if (propDef.apply) {
// Use type assertion to satisfy the type checker
propDef.apply(instance, props, key as string);
} else {
try {
(instance as Record<string, unknown>)[key] = value;
} catch (error) {
console.error(`Error applying prop ${key}: ${error}`);
}
}
}
}
});
}
) {
Object.entries(props).forEach(([key, value]) => {
if (!(key in schema)) return;
const propDef = schema[key] as PropValidator<T[keyof T], InstanceType> | undefined;
if (!propDef || propDef.readOnly) return;

if (propDef.apply) {
propDef.apply(instance, props, key as string);
} else {
try {
(instance as Record<string, unknown>)[key] = value;
} catch (error) {
console.error(`Error applying prop ${key}:`, error);
}
}
});
}
Comment on lines 222 to +242
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The new readOnly guard in applyProps and the getter-only detection in createComponentDefinition are the core of this fix but have no test coverage. Given that the codebase has component-level tests (e.g., Screen.test.tsx, Script.test.tsx) and the fix is non-trivial, a test should be added to verify that: (1) a component schema entry for a getter-only prop has readOnly: true (for primitive/Mat4/Material types), (2) applyProps does not throw or attempt to assign when a schema entry has readOnly: true, and (3) the original failing scenario (passing a custom asset to <GSplat />) works without throwing.

Copilot uses AI. Check for mistakes.


Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

There is a spurious trailing blank line with leading whitespace at line 244, immediately after the closing brace of applyProps. This stray indented blank line is inconsistent with the rest of the file's formatting and should be removed.

Suggested change

Copilot uses AI. Check for mistakes.
/**
* Property information including whether it's defined with a setter.
*/
export type PropertyInfo = {
value: unknown;
isDefinedWithSetter: boolean;
};
readOnly?: boolean; // Mark properties that should not be assigned
};
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The closing brace of the PropertyInfo type is indented with 2 spaces ( };) while the type body uses 4-space indentation. This is inconsistent with the rest of the file which uses 4-space indentation for type declarations. The closing brace should be at column 0, consistent with how PropValidator and Schema types are defined in this file.

Suggested change
};
};

Copilot uses AI. Check for mistakes.
Copy link
Author

@No-LAN-B No-LAN-B Mar 6, 2026

Choose a reason for hiding this comment

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

I normally have a lint script run on commit so I must have simply forgot to run the linter explicitly here should be a simple linter run to adapt some of the inline consistency.


/**
* Get the pseudo public props of an instance with setter information. This is useful for creating a component definition from an instance.
Expand Down Expand Up @@ -283,7 +283,7 @@ export function getPseudoPublicProps(container: Record<string, unknown>): Record
const hasGetter = typeof descriptor.get === 'function';
const hasSetter = typeof descriptor.set === 'function';

if (hasSetter && !hasGetter) return;
if (hasSetter && !hasGetter) return; // Only setter-only props are skipped

// If it's a getter/setter property, try to get the value
if (descriptor.get) {
Expand Down Expand Up @@ -369,8 +369,18 @@ export function createComponentDefinition<T, InstanceType>(

// Basic type detection
entries.forEach(([key, propertyInfo]) => {
if(exclude.includes(String(key))) return;
const { value, isDefinedWithSetter } = propertyInfo;
if (exclude.includes(String(key))) return;

// Mark getter-only properties as read-only
const descriptor = Object.getOwnPropertyDescriptor(
(instance as any).constructor.prototype,
key as string | symbol
);
if (descriptor && descriptor.get && !descriptor.set) {
propertyInfo.readOnly = true;
}
Comment on lines +374 to +381
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The getter-only detection in createComponentDefinition uses Object.getOwnPropertyDescriptor((instance as any).constructor.prototype, key), which only looks at the direct prototype of the constructor. If a getter-only property is inherited from a grandparent class (not defined on the immediate prototype but on a higher ancestor), this check will return undefined and the property will not be marked as readOnly.

Note that getPseudoPublicProps has the same limitation (only traverses one prototype level), so such a property would likely not be included in the schema at all — meaning the current code is consistent. However, this is a latent bug: if getPseudoPublicProps is ever extended to walk the full prototype chain, the getter-only detection here would need to be updated to match. Consider using a prototype-walking loop (similar to how getPseudoPublicProps could be extended) or using Object.getOwnPropertyDescriptor on each level of the prototype chain to be safe.

Copilot uses AI. Check for mistakes.
Copy link
Author

@No-LAN-B No-LAN-B Mar 6, 2026

Choose a reason for hiding this comment

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

This was actually one concern with integration and why I decided to go with early exits at the beginning. I would personally suggest downstream making a test case for grandparent classes ( if its a point of interest ) and simply run it to detect any fault lines in the class structure for the future.

As of writing this there are ZERO explicit grandparent classes in the traditional inheritance sense for the schema generator. Instead, the current system uses a composition-based pattern with type definitions and factory functions. So this is a misguided concern and Copilot didn't really do its due diligence here.


const { value, isDefinedWithSetter } = propertyInfo;

// Colors
if (value instanceof Color) {
Expand All @@ -380,6 +390,9 @@ export function createComponentDefinition<T, InstanceType>(
errorMsg: (val: unknown) => `Invalid value for prop "${String(key)}": "${val}". ` +
`Expected a hex like "#FF0000", CSS color name like "red", or an array "[1, 0, 0]").`,
apply: (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
if(typeof props[key] === 'string') {
const colorString = getColorFromName(props[key] as string) || props[key] as string;
(instance[key as keyof InstanceType] as Color) = new Color().fromString(colorString);
Expand All @@ -397,8 +410,14 @@ export function createComponentDefinition<T, InstanceType>(
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${JSON.stringify(val)}". ` +
`Expected an array of 2 numbers.`,
apply: isDefinedWithSetter ? (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as Vec2) = new Vec2().fromArray(props[key] as number[]);
} : (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as Vec2).set(...props[key] as [number, number]);
}
};
Expand All @@ -411,8 +430,14 @@ export function createComponentDefinition<T, InstanceType>(
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${JSON.stringify(val)}". ` +
`Expected an array of 3 numbers.`,
apply: isDefinedWithSetter ? (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as Vec3) = new Vec3().fromArray(props[key] as number[]);
} : (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as Vec3).set(...props[key] as [number, number, number]);
}
};
Expand All @@ -424,8 +449,14 @@ export function createComponentDefinition<T, InstanceType>(
default: [value.x, value.y, value.z, value.w],
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${JSON.stringify(val)}". Expected an array of 4 numbers.`,
apply: isDefinedWithSetter ? (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as Vec4) = new Vec4().fromArray(props[key] as number[]);
} : (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as Vec4).set(...props[key] as [number, number, number, number]);
}
};
Expand All @@ -439,8 +470,14 @@ export function createComponentDefinition<T, InstanceType>(
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${JSON.stringify(val)}". ` +
`Expected an array of 4 numbers.`,
apply: isDefinedWithSetter ? (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as Quat) = new Quat().fromArray(props[key] as number[]);
} : (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as Quat).set(...props[key] as [number, number, number, number]);
}
};
Expand All @@ -452,30 +489,34 @@ export function createComponentDefinition<T, InstanceType>(
default: Array.from((value.data)),
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${JSON.stringify(val)}". ` +
`Expected an array of 16 numbers.`,
...(propertyInfo.readOnly && { readOnly: true }),
};
}
// Numbers
else if (typeof value === 'number') {
schema[key] = {
validate: (val) => typeof val === 'number',
default: value,
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${val}". Expected a number.`
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${val}". Expected a number.`,
...(propertyInfo.readOnly && { readOnly: true }),
};
}
// Strings
else if (typeof value === 'string') {
schema[key] = {
validate: (val) => typeof val === 'string',
default: value as string,
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${val}". Expected a string.`
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${val}". Expected a string.`,
...(propertyInfo.readOnly && { readOnly: true }),
};
}
// Booleans
else if (typeof value === 'boolean') {
schema[key] = {
validate: (val) => typeof val === 'boolean',
default: value as boolean,
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${val}". Expected a boolean.`
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${val}". Expected a boolean.`,
...(propertyInfo.readOnly && { readOnly: true }),
};
}

Expand All @@ -486,6 +527,9 @@ export function createComponentDefinition<T, InstanceType>(
default: value,
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${JSON.stringify(val)}". Expected an array.`,
apply: (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
// For arrays, use a different approach to avoid spread operator issues
const values = props[key] as unknown[];

Expand All @@ -505,6 +549,7 @@ export function createComponentDefinition<T, InstanceType>(
validate: (val) => val instanceof Material,
default: value,
errorMsg: (val) => `Invalid value for prop "${String(key)}": "${JSON.stringify(val)}". Expected a Material.`,
...(propertyInfo.readOnly && { readOnly: true }),
};
}

Expand All @@ -515,6 +560,9 @@ export function createComponentDefinition<T, InstanceType>(
default: value,
errorMsg: () => '',
apply: (instance, props, key) => {
if (propertyInfo.readOnly) {
return;
}
(instance[key as keyof InstanceType] as unknown) = props[key];
}
};
Comment on lines 386 to 568
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

For schema entries that have an apply callback (Color, Vec2, Vec3, Vec4, Quat, Array, Null), the readOnly flag is not set on the schema entry itself — unlike types without an apply callback (Mat4, number, string, boolean, Material) which correctly spread ...(propertyInfo.readOnly && { readOnly: true }).

This inconsistency means:

  1. The outer guard in applyProps (if (!propDef || propDef.readOnly) return) will never skip these props at the top level even when the property is getter-only, because propDef.readOnly is undefined for these types.
  2. Any external code inspecting propDef.readOnly to determine if a prop is read-only (e.g. for docs/tooling) will get incorrect results for getter-only Color, Vec2, Vec3, Vec4, Quat, Array, or Null props.

The early-return inside the apply callback does prevent the actual write, so no exception is thrown — but the approach is not consistent with the rest of the implementation. The schema entries for these types should also spread ...(propertyInfo.readOnly && { readOnly: true }) so propDef.readOnly is reliable as a public indicator of read-only status.

Copilot uses AI. Check for mistakes.
Copy link
Author

@No-LAN-B No-LAN-B Mar 6, 2026

Choose a reason for hiding this comment

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

The main goal was to implement early returns as I did not want to disrupt any preexisting code that may have rely on the type failure to catch I couldn't find anything that did this but my aim was to make the change with as little potential ripples as possible. The early exit is the simplest and easiest fix besides simply skipping getter only properties but, I wanted to provide a read-only property implementation as a proof of concept for further development. For types without an apply callback implementing the read-only property for consistency made sense by doing a simple append to the schema as the previous logic was based off this universal 'apply all' idea which is flawed.

This boils down to 2 things:

  • Firstly the segment of code addressing apply callbacks is the essential bugfix here and I can definitely see fully implementing the read-only property to the schema being beneficial. It likely would become a much bigger change that I am willing to undertake but wanted to wait for approval as insurance.

  • Secondly the read-only being a public indicator would allow for both custom debugging / a more informative error message so rather than 'is getter only' pops up something like property is of read only or more informative messages involving class access can be provided through the property as a point of reference.

NOTE: Testing can and should use this read-only property especially for asset creation it is likely required to revisit some older tests and add some insurance guards. I ran the test module and everything passed quite nicely however it passing nicely also likely means that the testing scripts should be more strict with class access properties as support for flexibility of asset expression IMO should be a test in and of itself as the original engine addresses custom assets cleanly and as a wrapper for the engine should ensure that feature carries over.

Expand Down