Skip to content

Commit

Permalink
breaking: update types of accessors in preparation for the upcoming v…
Browse files Browse the repository at this point in the history
…ariant accessor types in TypeScript

This make accessors have the correct types to represent all possible values that they can be set to, but the getters are funky because to use them it requires type casting. As a temporary workaround, `get*()` methods have been added (f.e. `element.getPosition()` returns the same as `element.position` but with the value casted to the underlying object type.

This does not impact JavaScript users. They can continue to do things like `element.position.x = 123`. But TypeScript users can't do that. They would have to write `;(element.position as XYZNumberValues).x = 123` which is cumbersome, but the temporary getter methods allow TypeScript users to write `element.getPosition().x = 123` instead. TypeScript users can still set values as usual: `this.position = [1, 2, 3]` just like JavaScript users.

The upcoming changes in microsoft/TypeScript#42425 will allow us to update the getter types so TypeScript users can write `element.position.x = 123`.
  • Loading branch information
trusktr committed Feb 17, 2021
1 parent 8af9b93 commit 687c573
Show file tree
Hide file tree
Showing 11 changed files with 171 additions and 175 deletions.
2 changes: 1 addition & 1 deletion packages/lume/src/components/CameraRig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class CameraRig extends Node {
this.scrollFling!.y

untrack(() => {
this.cam!.position.z = this.scrollFling!.y
this.cam!.getPosition().z = this.scrollFling!.y
})
})
}
Expand Down
6 changes: 3 additions & 3 deletions packages/lume/src/components/Cube.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ export default class Cube extends Node {
// rotate and place each side.
if (index < 4)
// 4 sides
rotator.rotation.y = 90 * index
rotator.getRotation().y = 90 * index
// top/bottom
else rotator.rotation.x = 90 * (index % 2 ? -1 : 1)
else rotator.getRotation().x = 90 * (index % 2 ? -1 : 1)

side.position.z = this.size.x / 2
side.getPosition().z = this.getSize().x / 2

this.add(rotator)
}
Expand Down
48 changes: 22 additions & 26 deletions packages/lume/src/core/ImperativeBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,12 @@ function ImperativeBaseMixin<T extends Constructor>(Base: T) {

untrack(() => {
if (
this.sizeMode.x === 'proportional' ||
this.sizeMode.y === 'proportional' ||
this.sizeMode.z === 'proportional' ||
this.align.x !== 0 ||
this.align.y !== 0 ||
this.align.z !== 0
this.getSizeMode().x === 'proportional' ||
this.getSizeMode().y === 'proportional' ||
this.getSizeMode().z === 'proportional' ||
this.getAlign().x !== 0 ||
this.getAlign().y !== 0 ||
this.getAlign().z !== 0
) {
this._calcSize()
this.needsUpdate()
Expand Down Expand Up @@ -703,8 +703,10 @@ function ImperativeBaseMixin<T extends Constructor>(Base: T) {
* move _calcSize to a render task.
*/
protected _calculateMatrix(): void {
// const {__align: align, __mountPoint: mountPoint, __position: position, __origin: origin} = this
const {align, mountPoint, position, origin} = this
const align = this.getAlign()
const mountPoint = this.getMountPoint()
const position = this.getPosition()
const origin = this.getOrigin()

const size = this.calculatedSize

Expand Down Expand Up @@ -815,6 +817,8 @@ function ImperativeBaseMixin<T extends Constructor>(Base: T) {
}

protected _updateRotation(): void {
const {x, y, z} = this.getRotation()

// Currently rotation is left-handed as far as values inputted into
// the LUME APIs. This method converts them to Three's right-handed
// system.
Expand All @@ -831,34 +835,26 @@ function ImperativeBaseMixin<T extends Constructor>(Base: T) {
// TODO Make the handedness configurable (f.e. left handed or right
// handed rotation)

this.three.rotation.set(
-toRadians(this.rotation.x),
// We don't negate Y rotation here, but we negate Y translation
// in _calculateMatrix so that it has the same effect.
toRadians(this.rotation.y),
-toRadians(this.rotation.z),
)
// We don't negate Y rotation here, but we negate Y translation
// in _calculateMatrix so that it has the same effect.
this.three.rotation.set(-toRadians(x), toRadians(y), -toRadians(z))

// TODO Besides that Transformable shouldn't know about Three.js
// objects, it should also not know about Scene. The isScene check
// prevents us from having to import Scene (avoiding a circular
// dependency).
const childOfScene = (this.parent as any)?.isScene
const childOfScene = this.parent?.isScene

// TODO write a comment as to why we needed the childOfScne check to
// alternate rotation directions here. It's been a while, I forgot
// why. I should've left a comment when I wrote this!
this.threeCSS.rotation.set(
(childOfScene ? -1 : 1) * toRadians(this.rotation.x),
toRadians(this.rotation.y),
(childOfScene ? -1 : 1) * toRadians(this.rotation.z),
(childOfScene ? -1 : 1) * toRadians(x),
toRadians(y),
(childOfScene ? -1 : 1) * toRadians(z),
)
}

protected _updateScale(): void {
this.three.scale.set(this.scale.x, this.scale.y, this.scale.z)

this.threeCSS.scale.set(this.scale.x, this.scale.y, this.scale.z)
const {x, y, z} = this.getScale()
this.three.scale.set(x, y, z)
this.threeCSS.scale.set(x, y, z)
}

protected _calculateWorldMatricesInSubtree(): void {
Expand Down
44 changes: 22 additions & 22 deletions packages/lume/src/core/Node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,35 @@ describe('Node', () => {
it('default values', async () => {
const n = new Node()

expect(n.position.x).toEqual(0)
expect(n.position.y).toEqual(0)
expect(n.position.z).toEqual(0)
expect(n.getPosition().x).toEqual(0)
expect(n.getPosition().y).toEqual(0)
expect(n.getPosition().z).toEqual(0)

expect(n.rotation.x).toEqual(0)
expect(n.rotation.y).toEqual(0)
expect(n.rotation.z).toEqual(0)
expect(n.getRotation().x).toEqual(0)
expect(n.getRotation().y).toEqual(0)
expect(n.getRotation().z).toEqual(0)

expect(n.scale.x).toEqual(1)
expect(n.scale.y).toEqual(1)
expect(n.scale.z).toEqual(1)
expect(n.getScale().x).toEqual(1)
expect(n.getScale().y).toEqual(1)
expect(n.getScale().z).toEqual(1)

expect(n.align.x).toEqual(0)
expect(n.align.y).toEqual(0)
expect(n.align.z).toEqual(0)
expect(n.getAlign().x).toEqual(0)
expect(n.getAlign().y).toEqual(0)
expect(n.getAlign().z).toEqual(0)

expect(n.mountPoint.x).toEqual(0)
expect(n.mountPoint.y).toEqual(0)
expect(n.mountPoint.z).toEqual(0)
expect(n.getMountPoint().x).toEqual(0)
expect(n.getMountPoint().y).toEqual(0)
expect(n.getMountPoint().z).toEqual(0)

expect(n.opacity).toEqual(1)
expect(n.getOpacity()).toEqual(1)

expect(n.size.x).toEqual(0, 'default size value not as expected')
expect(n.size.y).toEqual(0, 'default size value not as expected')
expect(n.size.z).toEqual(0, 'default size value not as expected')
expect(n.getSize().x).toEqual(0, 'default size value not as expected')
expect(n.getSize().y).toEqual(0, 'default size value not as expected')
expect(n.getSize().z).toEqual(0, 'default size value not as expected')

expect(n.sizeMode.x).toEqual('literal')
expect(n.sizeMode.y).toEqual('literal')
expect(n.sizeMode.z).toEqual('literal')
expect(n.getSizeMode().x).toEqual('literal')
expect(n.getSizeMode().y).toEqual('literal')
expect(n.getSizeMode().z).toEqual('literal')
})

it('element is an instance of Node, created with `new`', async () => {
Expand Down
1 change: 1 addition & 0 deletions packages/lume/src/core/Node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {default as HTMLInterface} from '../html/HTMLNode.js'
// register behaviors that can be used on this element
import '../html/behaviors/ObjModelBehavior.js'
import '../html/behaviors/GltfModelBehavior.js'
import '../html/behaviors/ColladaModelBehavior.js'

import type {BaseAttributes} from './ImperativeBase.js'

Expand Down
6 changes: 3 additions & 3 deletions packages/lume/src/core/Scene.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ function SceneMixin<T extends Constructor>(Base: T) {
* [`Sizeable.sizeMode`](TODO) property to make the default values for the X and
* Y axes both "proportional".
*/
this.sizeMode.set('proportional', 'proportional', 'literal')
this.getSizeMode().set('proportional', 'proportional', 'literal')

/**
* @override
Expand All @@ -201,7 +201,7 @@ function SceneMixin<T extends Constructor>(Base: T) {
* [`Sizeable.size`](TODO) property to make the default values for the
* X and Y axes both `1`.
*/
this.size.set(1, 1, 0)
this.getSize().set(1, 1, 0)

// The scene should always render CSS properties (it needs to always
// be rendered or resized, for example, because it contains the
Expand Down Expand Up @@ -654,7 +654,7 @@ function SceneMixin<T extends Constructor>(Base: T) {
// If we will be rendering something...
(this.enableCss || this.webgl) &&
// ...and if one size dimension is proportional...
(this.sizeMode.x == 'proportional' || this.sizeMode.y == 'proportional')
(this.getSizeMode().x == 'proportional' || this.getSizeMode().y == 'proportional')
// Note, we don't care about the Z dimension, because Scenes are flat surfaces.
) {
// ...then observe the parent element size (it may not be a LUME
Expand Down
86 changes: 51 additions & 35 deletions packages/lume/src/core/Sizeable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,10 @@ import Motor from './Motor.js'

import type {MixinResult} from 'lowclass'
import type {StopFunction} from '@lume/element'
import type {XYZValuesObject, XYZValuesArray, XYZPartialValuesArray, XYZPartialValuesObject} from './XYZValues.js'
import type {XYZValues, XYZValuesObject, XYZPartialValuesArray, XYZPartialValuesObject} from './XYZValues.js'
import type {SizeModeValue} from './XYZSizeModeValues.js'
import type {RenderTask} from './Motor.js'

// Property functions are used for animating properties of type XYZNumberValues or XYZNonNegativeValues
type XYZPropertyFunction = (
x: number,
y: number,
z: number,
time: number,
) => XYZValuesObject<number> | XYZValuesArray<number> | false

type SinglePropertyFunction = (value: number, time: number) => number | false
import type {XYZNumberValues} from './XYZNumberValues.js'

// Cache variables to avoid making new variables in repeatedly-called methods.
const previousSize: Partial<XYZValuesObject<number>> = {}
Expand All @@ -40,8 +31,12 @@ function SizeableMixin<T extends Constructor<HTMLElement>>(Base: T) {
constructor(...args: any[]) {
super(...args)

this.sizeMode.on('valuechanged', () => !this._isSettingProperty && (this.sizeMode = this.sizeMode))
this.size.on('valuechanged', () => !this._isSettingProperty && (this.size = this.size))
// TODO: Once TS lands the upcoming feature to have getters with
// different types than setters, we can fix the ugly type casting
// in the following constructor lines.
// Tracking issues: https://github.com/microsoft/TypeScript/issues/2521 and https://github.com/microsoft/TypeScript/pull/42425
this.getSizeMode().on('valuechanged', () => !this._isSettingProperty && (this.sizeMode = this.sizeMode))
this.getSize().on('valuechanged', () => !this._isSettingProperty && (this.size = this.size))
}

/**
Expand All @@ -58,18 +53,21 @@ function SizeableMixin<T extends Constructor<HTMLElement>>(Base: T) {
*/
@attribute
@emits('propertychange')
set sizeMode(newValue) {
set sizeMode(newValue: XYZSizeModeValuesProperty) {
if (typeof newValue === 'function') throw new TypeError('property functions are not allowed for sizeMode')
if (!this.__sizeMode) this.__sizeMode = new XYZSizeModeValues('literal', 'literal', 'literal')
this._setPropertyXYZ('sizeMode', newValue)
}
get sizeMode() {
get sizeMode(): XYZSizeModeValuesProperty {
if (!this.__sizeMode) this.__sizeMode = new XYZSizeModeValues('literal', 'literal', 'literal')
return this.__sizeMode
}

private declare __sizeMode?: XYZSizeModeValues

// prettier-ignore
getSizeMode(): XYZSizeModeValues { return this.sizeMode as XYZSizeModeValues }

// TODO: A "differential" size would be cool. Good for padding,
// borders, etc. Inspired from Famous' differential sizing.
//
Expand Down Expand Up @@ -100,17 +98,20 @@ function SizeableMixin<T extends Constructor<HTMLElement>>(Base: T) {
*/
@attribute
@emits('propertychange')
set size(newValue) {
set size(newValue: XYZNonNegativeNumberValuesProperty | XYZNonNegativeNumberValuesPropertyFunction) {
if (!this.__size) this.__size = new XYZNonNegativeValues(0, 0, 0)
this._setPropertyXYZ('size', newValue)
}
get size() {
get size(): XYZNonNegativeNumberValuesProperty | XYZNonNegativeNumberValuesPropertyFunction {
if (!this.__size) this.__size = new XYZNonNegativeValues(0, 0, 0)
return this.__size
}

private declare __size?: XYZNonNegativeValues

// prettier-ignore
getSize(): XYZNonNegativeValues { return this.size as XYZNonNegativeValues }

/**
* Get the actual size of the Node. This can be useful when size is
* proportional, as the actual size of the Node depends on the size of
Expand Down Expand Up @@ -194,7 +195,8 @@ function SizeableMixin<T extends Constructor<HTMLElement>>(Base: T) {

Object.assign(previousSize, calculatedSize)

const {sizeMode, size} = this
const size = this.getSize()
const sizeMode = this.getSizeMode()
const parentSize = this._getParentSize()

if (sizeMode.x == 'literal') {
Expand Down Expand Up @@ -282,7 +284,7 @@ function SizeableMixin<T extends Constructor<HTMLElement>>(Base: T) {
private __propertyFunctions: Map<string, RenderTask> | null = null
private __settingValueFromPropFunction = false

private __handleXYZPropertyFunction(fn: XYZPropertyFunction, name: keyof this) {
private __handleXYZPropertyFunction(fn: XYZNumberValuesPropertyFunction, name: keyof this) {
if (!this.__propertyFunctions) this.__propertyFunctions = new Map()

const propFunction = this.__propertyFunctions.get(name as string)
Expand Down Expand Up @@ -361,33 +363,47 @@ function SizeableMixin<T extends Constructor<HTMLElement>>(Base: T) {
return Sizeable as MixinResult<typeof Sizeable, T>
}

export const Sizeable = Mixin(SizeableMixin)
export interface Sizeable extends InstanceType<typeof Sizeable> {}
export default Sizeable

// the following type guards are used above just to satisfy the type system,
// though the actual runtime check does not guarantee that the functions are of
// the expected shape.

function isXYZPropertyFunction(f: any): f is XYZPropertyFunction {
function isXYZPropertyFunction(f: any): f is XYZNumberValuesPropertyFunction {
return typeof f === 'function'
}

function isSinglePropertyFunction(f: any): f is SinglePropertyFunction {
return typeof f === 'function'
}

export const Sizeable = Mixin(SizeableMixin)
export interface Sizeable extends InstanceType<typeof Sizeable> {}
export default Sizeable

export type Size = XYZNonNegativeValues | XYZPartialValuesArray<number> | XYZPartialValuesObject<number> | string
export type SizeMode =
| XYZSizeModeValues
| XYZPartialValuesArray<SizeModeValue>
| XYZPartialValuesObject<SizeModeValue>
// This type represents the types of values that can be set via attributes or
// properties (attributes pass strings to properties and properties all handle
// string values for example, hence why it includes `| string`)
export type XYZValuesProperty<XYZValuesType extends XYZValues, DataType> =
| XYZValuesType
| XYZPartialValuesArray<DataType>
| XYZPartialValuesObject<DataType>
| string

export function size(val: Size) {
return val as XYZNonNegativeValues
}
export type XYZNumberValuesProperty = XYZValuesProperty<XYZNumberValues, number>
export type XYZNonNegativeNumberValuesProperty = XYZValuesProperty<XYZNonNegativeValues, number>
export type XYZSizeModeValuesProperty = XYZValuesProperty<XYZSizeModeValues, SizeModeValue>

export function sizeMode(val: SizeMode) {
return val as XYZSizeModeValues
}
// Property functions are used for animating properties of type XYZNumberValues
export type XYZValuesPropertyFunction<XYZValuesPropertyType, DataType> = (
x: DataType,
y: DataType,
z: DataType,
time: number,
) => XYZValuesPropertyType | false

export type XYZNumberValuesPropertyFunction = XYZValuesPropertyFunction<XYZNumberValuesProperty, number>
export type XYZNonNegativeNumberValuesPropertyFunction = XYZValuesPropertyFunction<
XYZNonNegativeNumberValuesProperty,
number
>

export type SinglePropertyFunction = (value: number, time: number) => number | false

0 comments on commit 687c573

Please sign in to comment.