fix: Do not applyProps if value has not changed#298
Conversation
|
commit: |
There was a problem hiding this comment.
Pull request overview
This PR introduces an optimization to prevent unnecessary prop applications that trigger setter side effects, specifically addressing issue #297 where the GSplat component's unified flag couldn't be changed after the component was enabled.
- Adds a
valuesEqualutility function that handles equality checking for primitives and PlayCanvas objects (Vec2, Vec3, Color, etc.) with support for approximate floating-point comparisons - Modifies
applyPropsto skip property application when values haven't changed, avoiding unwanted side effects from setters - Refactors
shallowEqualsto use the newvaluesEqualfunction for more robust property comparisons
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/lib/src/utils/compare.tsx | Introduces the new valuesEqual function with support for PlayCanvas object equality checking via equalsApprox and equals methods; updates shallowEquals to leverage this new function |
| packages/lib/src/utils/validation.ts | Integrates valuesEqual into applyProps to check if property values have changed before applying them, preventing unnecessary setter invocations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const currentValue = (instance as Record<string, unknown>)[key]; | ||
|
|
||
| // Skip if value hasn't changed (avoids side effects from setters) | ||
| if (valuesEqual(currentValue, value)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The equality check compares the current instance property value with the new prop value, but this comparison may be incorrect when a custom apply function exists. The apply function often transforms the input (e.g., an array [1, 2, 3] gets transformed into a Vec3 object). This means you're comparing a Vec3 object with an array, which will always return false and the optimization won't work.
Consider checking equality after the transformation, or store the last applied raw value to compare against the new raw value before transformation.
There was a problem hiding this comment.
I think that is a good suggestion, will try to rework this
| export function valuesEqual(a: unknown, b: unknown): boolean { | ||
| if (a === b) return true; | ||
|
|
||
| // Early exit if either is null/undefined (using type coercion) | ||
| if (a == null || b == null) return false; | ||
|
|
||
| if (typeof a === 'object') { | ||
| // Priority 1: Floating point approximation (handles precision drift) | ||
| if ('equalsApprox' in a && typeof (a as Approximate).equalsApprox === 'function') { | ||
| return (a as Approximate).equalsApprox(b); | ||
| } | ||
|
|
||
| // Priority 2: Strict structural equality | ||
| if ('equals' in a && typeof (a as Equatable).equals === 'function') { | ||
| return (a as Equatable).equals(b); | ||
| } | ||
| } | ||
|
|
||
| // For other objects, return false to trigger re-apply (conservative) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The new valuesEqual function lacks test coverage. Given the critical nature of this function (it's used to prevent unnecessary prop applications and avoid side effects), it should have comprehensive tests covering:
- Primitive comparisons (numbers, strings, booleans, null, undefined)
- Object comparisons using equalsApprox (Vec2, Vec3, Vec4, Color, Quat)
- Object comparisons using equals
- Edge cases like comparing objects with arrays
Consider adding a test file similar to picker.test.tsx to ensure this function behaves correctly in all scenarios.
There was a problem hiding this comment.
@abstrakt8 while I can't really help with the refactor mentioned in the other thread, here is at least a test file to add coverage for valuesEqual (in hopes of assisting this PR to get merged faster):
compare.test.ts
| equalsApprox(other: unknown): boolean; | ||
| } | ||
|
|
||
| interface Equatable { |
There was a problem hiding this comment.
Don't think Equatable is quite right... Exact maybe?
The gsplat model disappears when props change, or rerenders occur (see playcanvas/react#302). Until playcanvas fixes this issue (reference PR playcanvas/react#298), change the react key for `SplatModel` so that it "refetches" the model and rerenders it that way. This causes a temporary blink of the model but it is short because the BE now sends caching headers (terraware/terraware-server#3848).
| // Check if all keys and their values are equal using valuesEqual | ||
| for (let i = 0; i < keysA.length; i++) { | ||
| const key = keysA[i]; | ||
| const propA = objA[key]; | ||
| const propB = objB[key]; | ||
| // If the object has an equality operator, use this | ||
| if(hasEqualsMethod(propA)) { | ||
| return propA.equals(propB); | ||
| } else if (propA !== propB) { | ||
| if (!valuesEqual(objA[key], objB[key])) { |
There was a problem hiding this comment.
There's a minor bug here because of the keys length check, though it may not matter much in use cases (feel free to resolve):
If objA has a property that has a value passed of undefined, and objB does not have that property at all but has a different property, then the keys length check will return true, and this for loop will not catch that objB has a property with a value that objA does not have. Example:
let objA = {foo: 'bar', baz: undefined};
let objB = {foo: 'bar', other: 'value'};
...
objA['baz'] === objB['baz'] // this returns true even though objB does not have the property bazThis may not matter in practice though, if this is only used for objects of a component. In that case, ignore this comment.
If this is an issue though, just change the length check to checking the actual strings check:
if (keysA !== keysB) { // (may have to use `toSorted` first)
|
If others run across this PR and have the same issue, we were able to fix some of the issues with React's import { memo } from 'react';
const SplatMode = ({splatSrc}: {splatSrc: string}) => {
const { asset } = useSplat(splatSrc);
return (
<Entity name='splat'>
<GSplat asset={asset} />
</Entity>
)
};
// key memo usage here:
export default memo(SplatModel);Obviously ymmv and that may depend on what version of React you're on. I'll also mention that we're using the React Compiler in case that affects this. |
What has changed?
Certain setters actually have side effects (especially in the
GSplatComponent) and we want to avoid triggering these side effects if the value has not changed. Examples:This can easily happen whenever we need to re-render a GSplat component and all the props get re-applied again. This can cause undesired effects.