Skip to content
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.

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

Add transform to control.ts and unit tests #13

Merged
merged 59 commits into from
Jun 20, 2024
Merged

Conversation

neverbiasu
Copy link
Contributor

@neverbiasu neverbiasu commented Apr 2, 2024

This PR implemented 2 of css 2D transform-function(rotate and translateX), and also provided unit tests and integration tests.

Copy link
Contributor Author

@neverbiasu neverbiasu left a comment

Choose a reason for hiding this comment

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

some format issues

@@ -5,9 +5,18 @@
@material liteblue {
diffuse-color: blue;
}
@keyframes rotateSelf {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove it

loadBuiltinPanel(
'spatial-devtools/console.xsml', nativeDocument, new BABYLON.Vector3(0, -0.8, -2)),
]);
// panels = await Promise.all([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove comments


expect(result).toEqual(expectedMatrix);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
});
});

@@ -311,6 +315,25 @@ export class InteractiveDynamicTexture extends BABYLON.DynamicTexture {
isDirtyAfterRendering = control.isDirty();
} else {
layout = currentElementOrControl._control.layoutNode.getLayout();
const control = currentElementOrControl._control;
const style = control._style;
// console.log('style', style);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove comments

<div class="inner-rect"></div>
</div>
</body>
</html>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
</html>
</html>

@@ -370,7 +392,7 @@ export class Control2D {
const canvasContext = this._renderingContext;
const boxRect = new DOMRectImpl(x, y, width, height);
const hasTextChildren = this._isElementOwnsInnerText();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change

@@ -135,6 +135,7 @@ export default class HTMLImageElementImpl extends HTMLContentElement implements

private async _resizeImageData() {
if (this._enableResizing && this._imageBitmap) {
console.log('height', this.height);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove comments

</script>
</space>
</xsml>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change

fixtures/css/css-transform.xsml Outdated Show resolved Hide resolved
</style>
</head>
<space>
<plane id="plane1">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
<plane id="plane1">
<plane>

this.transform = new DOMMatrix(args);
} else if (args.length === 1 && typeof args[0] === 'object') {
const transformInit: DOMMatrix2DInit = args[0];
this.transform = new DOMMatrix([transformInit.m11, transformInit.m12, 0, 0, transformInit.m21, transformInit.m22, 0, 0, 0, 0, 1, 0, transformInit.m41, transformInit.m42, 0, 1]); // change m32 m33 to pass the test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
this.transform = new DOMMatrix([transformInit.m11, transformInit.m12, 0, 0, transformInit.m21, transformInit.m22, 0, 0, 0, 0, 1, 0, transformInit.m41, transformInit.m42, 0, 1]); // change m32 m33 to pass the test
this.transform = new DOMMatrix([
transformInit.m11, transformInit.m12, 0, 0,
transformInit.m21, transformInit.m22, 0, 0,
0, 0, 1, 0,
transformInit.m41, transformInit.m42, 0, 1
]);

reviseCode
reviseFormat
console.log('mouseY', mouseY);
const x = Math.round(mouseX);
img.style.transform = `translateX(${x - 512}px)`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change

]);
matrix = postMultiply(matrix, translateMatrix);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change

this._renderingContext = mockContext as any;
}
prepareTransformMatrix(matrix: DOMMatrix) {
this.transform = matrix;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
this.transform = matrix;
this.transform = matrix;

this.transform = matrix;
}
expect(matrix: DOMMatrix) {
expect(this._renderingContext.getTransform()).toEqual(matrix);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
expect(this._renderingContext.getTransform()).toEqual(matrix);
expect(this._renderingContext.getTransform()).toEqual(matrix);

}
</style>
<div>
<img src="./texture.jpg">
Copy link
Contributor

Choose a reason for hiding this comment

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

The texture file should moved to fixtures/textures directory.

<img src="./texture.jpg">
</div>
</plane>
<cube class="m_cube" />
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this cube for? It seems not related with CSS transform :(

@@ -0,0 +1,30 @@
import { InteractiveDynamicTexture } from './InteractiveDynamicTexture';
import DOMMatrix from '../../geometry/DOMMatrix'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import DOMMatrix from '../../geometry/DOMMatrix'
import DOMMatrix from '../../geometry/DOMMatrix';

@@ -0,0 +1,30 @@
import { InteractiveDynamicTexture } from './InteractiveDynamicTexture';
import DOMMatrix from '../../geometry/DOMMatrix'
import { describe, it, expect } from '@jest/globals'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { describe, it, expect } from '@jest/globals'
import { describe, it, expect } from '@jest/globals';


describe('InteractiveDynamicTexture', () => {
it('should return correct matrix', () => {
const transformStr = "rotate(90deg) translateX(10px)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const transformStr = "rotate(90deg) translateX(10px)"
const transformStr = 'rotate(90deg) translateX(10px)';

expect(result).toEqual(expectedMatrix);
})
it('should return identity matrix when no transform is applied', () => {
const transformStr = " "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const transformStr = " "
const transformStr = ' ';

private _overwriteHeight: number;
private _overwriteWidth: number;
private _imageData: ImageDataImpl;
private _imageData: ImageBitmap;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Private field should be a *impl type
  2. The name should be _imageBitmap if its type is ImageBitmap*

@@ -164,12 +163,13 @@ export class Control2D {
private _lastRect: DOMRectReadOnlyImpl;
private _lastCursor: BABYLON.Vector2;
private _isCursorInside = false;
private _renderingContext: CanvasRenderingContext2D;
protected _renderingContext: CanvasRenderingContext2D; // for unit test
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the comments that is unrelated.

private _isDirty = true;

private _transform: DOMMatrix;
private _currentTransform: DOMMatrix;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, for the above 2 fields.

@@ -197,7 +197,7 @@ export class Control2D {
}
}

setImageData(data: ImageDataImpl) {
setImageData(data: ImageBitmap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setImageData(data: ImageBitmap) {
setImageBitmap(bitmap: ImageBitmap) {

}


_updateTransform(renderingContext: CanvasRenderingContext2D = this._renderingContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to pass an argument of renderingContext, because it always be this._renderingContext, feel free to correct me if I'm wrong :)

@@ -337,6 +356,48 @@ export class InteractiveDynamicTexture extends BABYLON.DynamicTexture {
return isDirtyAfterRendering;
}

// cannot call it in `render()`, cuz it cost too much.
static _parserTransform(transformStr: string): DOMMatrix {
Copy link
Contributor

Choose a reason for hiding this comment

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

The CSS property parsing should not be a rendering-related source file, please do this work in CSS-related files.

@@ -337,6 +356,48 @@ export class InteractiveDynamicTexture extends BABYLON.DynamicTexture {
return isDirtyAfterRendering;
}

// cannot call it in `render()`, cuz it cost too much.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you does call this function in render() :(

src/living/helpers/gui2d/control.test.ts Show resolved Hide resolved
]);
control.prepareTransformMatrix(matrix);
control._updateTransform();
control.expect(matrix);
Copy link
Contributor

Choose a reason for hiding this comment

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

Mock class should do mocking only, expectation is not the scope of mock.

0, 0, 1, 0,
0, 10, 0, 1
]);
const result = InteractiveDynamicTexture._parserTransform(transformStr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, InteractiveDynamicTexture should not contain a function that does parsing, you have to finish it in CSS parsing stage in this project.

@@ -0,0 +1,24 @@
import DOMMatrixImpl from '../geometry/DOMMatrix';
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 functions seems to be moved in matrix-functions.ts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or change this source filename to "matrix-transform-functions.ts".

it('should process input function name error correctly', () => {
const result = parsers.parseTransform('translateM(10px)');
expect(result).toEqual([]);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
})
});

it('should process input values error correctly', () => {
const result = parsers.parseTransform('translateX(10py)');
expect(result).toEqual([]);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
})
});

@@ -18,6 +18,7 @@ const calcRegEx = /^calc\(([^)]*)\)$/;
const colorRegEx4 =
/^hsla?\(\s*(-?\d+|-?\d*.\d+)\s*,\s*(-?\d+|-?\d*.\d+)%\s*,\s*(-?\d+|-?\d*.\d+)%\s*(,\s*(-?\d+|-?\d*.\d+)\s*)?\)/;
const angleRegEx = /^([-+]?[0-9]*\.?[0-9]+)(deg|grad|rad)$/;
const transformFunctionRegEx = /(\w+)\((\w+)\)/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const transformFunctionRegEx = /(\w+)\((\w+)\)/g;
const css3TransformFunctionNames = [
'translate',
'translateX',
'translateY',
...
];
const transformFunctionRegEx = new RegExp(`(${css3TransformFunctionNames.join('|')})\(\w+\)`, 'g');


export class TranslationTransformFunction extends TransformFunction<PropertyLengthValue> {
constructor(name: string, args: string[]) {
const values = processArgs(args, toLengthStr) as PropertyLengthValue[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const values = processArgs(args, toLengthStr) as PropertyLengthValue[];
const values = super.processArgs(args, toLengthStr);

}

calculateTransformMatrix(transformFunctions: UnionTransformFunction[]): DOMMatrixImpl {
let transformMatrix = new DOMMatrixImpl([
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let transformMatrix = new DOMMatrixImpl([
let transformMatrix: DOMMatrix = new DOMMatrixImpl([

@@ -950,3 +989,110 @@ export function shorthandSetter(
}
};
}

function createAndAddTransformFunctionTo(
parsedTransformFunctions: UnionTransformFunction[],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parsedTransformFunctions: UnionTransformFunction[],
list: UnionTransformFunction[],

this.values = values;
}

static processArgs(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static processArgs(
static ProcessArgs(

@@ -370,11 +372,10 @@ export class Control2D {
const canvasContext = this._renderingContext;
const boxRect = new DOMRectImpl(x, y, width, height);
const hasTextChildren = this._isElementOwnsInnerText();

/**
* Check if this node is an image or has text children, if yes, we need to fix the size by the image or text.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Check if this node is an image or has text children, if yes, we need to fix the size by the image or text.
* 1. Check if this node is an image or has text children, if yes, we need to fix the size by the image or text.

const transformStr = style.transform;
const parentElement = element.parentElement;
const transformFunctions = parseTransform(transformStr);
const transformMatrix = this.calculateTransformMatrix(transformFunctions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const transformMatrix = this.calculateTransformMatrix(transformFunctions);
const transformMatrix = this.calculateTransformMatrix(parseTransform(transformStr));

@@ -15,7 +15,7 @@ export function postMultiply(self: DOMMatrix, other: DOMMatrix): DOMMatrix {
return new DOMMatrixImpl(resElements);
}

export function preMultiply(other: DOMMatrix, self: DOMMatrix): DOMMatrix {
export function preMultiply(other: DOMMatrix, self: DOMMatrix): DOMMatrix{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export function preMultiply(other: DOMMatrix, self: DOMMatrix): DOMMatrix{
export function preMultiply(other: DOMMatrix, self: DOMMatrix): DOMMatrix {

@@ -739,9 +755,56 @@ export class Control2D {
element.width = rect.width;
element.height = rect.height;
});
renderingContext.putImageData(this._imageData, rect.x, rect.y, 0, 0, rect.width, rect.height);
/**
* NOTE|feichi|: `putImageData` paints image onto canvas without transform,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* NOTE|feichi|: `putImageData` paints image onto canvas without transform,
* NOTE(feichi): `putImageData` paints image onto canvas without transform,

const parentElement = element.parentElement;
const transformMatrix = this.calculateTransformMatrix(parseTransform(transformStr));
/**
* If the parentElement is the direct childNodes of ShadowRoot, it will be null.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add an example to explain

@@ -739,9 +755,60 @@ export class Control2D {
element.width = rect.width;
element.height = rect.height;
});
renderingContext.putImageData(this._imageData, rect.x, rect.y, 0, 0, rect.width, rect.height);
/**
* NOTE(feichi): `putImageData` paints image onto canvas without transform,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not clear enough

Copy link
Contributor

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

LGTM

@yorkie yorkie merged commit 02172a0 into M-CreativeLab:main Jun 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants