Use `isThing` rather than instanceof; add typedefs and tests #55

ChristopherChudzicki · 2023-01-09T23:07:58Z

In #49 we discussed two changes:

simpling ThreeJS imports
swapping instanceof checks for isColor, isVector2, etc, that ThreeJS uses.

#53 implemented (1), this PR does (2).

Additionally, this PR contains type definitions for more properties. Yay.

ChristopherChudzicki · 2023-01-09T23:11:50Z

src/primitives/types/types_typed.ts

@@ -121,18 +144,60 @@ export type TypeGenerators = {
  object: any;
  timestamp: any;

-  vec2: any;


This is technically a breaking change, at least for TS users, I suppose.

When I initially added TS definitions, I only added full definitions for some properties, because I was still trying to understand the codebase (and still am, I suppose). I decided to leave untyped stuff as any rather than unknown, which would have been the safer but more annoying approach.

Anyway, improvements to static typing are always going to be somewhat breaking, so I don't think this warrants a major version bump.

I'm fine with that, we can go semver once we get the README tidied up with all of this extra dev info and with updates on the changes we've made to the project.

sritchie

This looks great, merge as you please, I just had a couple of questions inline.

sritchie · 2023-01-11T14:03:15Z

src/primitives/types/types_typed.ts

+ */
+type ColorDescription = string | Color | number | number[];
+
+type Vec2Like = number | number[] | Vector2;


what's the meaning of the number type for these? Do we interpret that as the first coordinate and fill in zeros for the rest? I get it for color above, but not yet for the Vec*Like types.

Basically, for vec2/3/4, supplying a single number behaves like supplying an array with one item. And if the array has fewer items than expected, default values are used for the remaining items.

Take for example <cartesian /> nodes... One of their traits is view3. The view3 trait defines several properties, [including scale](https://github.com/unconed/mathbox/blob/master/src/primitives/types/traits.ts#L40:

// the Type for <cartesian>'s "scale" property, as defined in traits.js scale: Types.vec3(1, 1, 1)

But imagine it was scale: Types.vec3(a, b, c)... Then:

cartesian.set("scale", x) // --> [x, b, c] cartesian.set("scale", [x, y]) // --> [x, y, c] cartesian.set("scale", [x, y, z]) // --> [x, y, z] cartesian.set("scale", new THREE.Vector3(x)) // --> [x, 0, 0] ... the 0, 0 defaults are set by THREE

I'm not trying to suggest that supplying a single number is actually a good idea... just documenting the current type validation system it seems intended.

sritchie · 2023-01-11T14:03:55Z

src/primitives/types/types_typed.ts

@@ -121,18 +144,60 @@ export type TypeGenerators = {
  object: any;
  timestamp: any;

-  vec2: any;


I'm fine with that, we can go semver once we get the README tidied up with all of this extra dev info and with updates on the changes we've made to the project.

sritchie · 2023-01-11T14:04:22Z

src/primitives/types/types_typed.ts

-  ivec3: any;
-  vec4: any;
-  ivec4: any;
+  vec2(x?: number, y?: number): Type<Vec2Like, Vector2>;


do we default values to 0?

It depends. There are multiple properties with type vec2, and each can have different defaults. Calling Types.vec2(x, y) is more like creating a validation instance, with instance-specific defaults.

Using view3 as an example again (which Caresian and some other nodes use):

// traits.js view3: { position: Types.vec3(), quaternion: Types.quat(), rotation: Types.vec3(), scale: Types.vec3(1, 1, 1), eulerOrder: Types.swizzle("xyz"), },

Calling Types.vec3(a,b,c) creates a sort of schema that (1) provides a default value of (a, b, c) and (2) provides a validator function.
So:

position's default value is (0, 0, 0) and when you set position, any unspecified components default to 0.

scale's default value is (1, 1, 1) and when you set scale, any unspecified components are set to 1.

sritchie · 2023-01-11T14:05:40Z

test/primitives/types/types.spec.ts

+     * but the defaults are usually not used. (They are when passing an array,
+     * but not when passing a Vector4 instance).
+     * 
+     * In pratice this probably doesn't really matter, since Mathbox doesn't


sritchie · 2023-01-11T14:06:06Z

test/primitives/types/types.spec.ts

+    ).toEqual(new Color(0.3,0.4,0.5));
+    expect(onInvalid).not.toHaveBeenCalled();
+
+    // Numbers are interpretted as hex


interpreted*

sritchie · 2023-01-11T14:07:37Z

test/primitives/types/types.spec.ts

+    /**
+     * This seems like a bug. `Types.quat(...)` accepts default values for xyzw,
+     * but the defaults are usually not used. (They are when passing an array,
+     * but not when passing a Vector4 instance).


that does seem like a bug! where does that happen? Can we make a ticket at least if there's some spot where the vector4 and array behaviors differ?

I reworded the comment a little. I think it's not currently observable, but would be easy to fix.

ChristopherChudzicki added 6 commits January 7, 2023 21:24

use isWhatever for runtime typechecks

41f54f6

temp

858db16

add typing + test for Types.color

33c7571

add tests for vec2

6bb00e3

add tests for vec3, vec4

393c5af

add quat tests

ff3ef21

ChristopherChudzicki force-pushed the cc/is branch from 5e37d89 to ff3ef21 Compare January 9, 2023 23:08

ChristopherChudzicki commented Jan 9, 2023

View reviewed changes

fix test

a72cfbf

ChristopherChudzicki requested a review from sritchie January 10, 2023 20:33

sritchie reviewed Jan 11, 2023

View reviewed changes

sritchie approved these changes Jan 11, 2023

View reviewed changes

sritchie linked an issue Jan 11, 2023 that may be closed by this pull request

Bug when using mathbox + THREE.Color in a bundle #49

Closed

fix types; clarify comment

1e5abc9

ChristopherChudzicki merged commit 558ffc2 into master Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `isThing` rather than instanceof; add typedefs and tests #55

Use `isThing` rather than instanceof; add typedefs and tests #55

ChristopherChudzicki commented Jan 9, 2023

ChristopherChudzicki Jan 9, 2023

sritchie Jan 11, 2023

sritchie left a comment

sritchie Jan 11, 2023

ChristopherChudzicki Jan 12, 2023

sritchie Jan 11, 2023

sritchie Jan 11, 2023

ChristopherChudzicki Jan 12, 2023

sritchie Jan 11, 2023

sritchie Jan 11, 2023

sritchie Jan 11, 2023

ChristopherChudzicki Jan 12, 2023

Use isThing rather than instanceof; add typedefs and tests #55

Use isThing rather than instanceof; add typedefs and tests #55

Conversation

ChristopherChudzicki commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sritchie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `isThing` rather than instanceof; add typedefs and tests #55

Use `isThing` rather than instanceof; add typedefs and tests #55