feat: shape component support multi-shape and stroke attribute #870
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates the shape rendering and timeline systems. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/effects-core/src/plugins/shape/build-line.ts (4)
9-63: Consider modularizing to improve readability.
Thesquarefunction is fairly small and self-contained. However, as more cap styles or variations are added, consider extracting shared logic (e.g., vector transformations) into a utility module to avoid duplication and keep this file focused.
142-166: Handle polygon orientation with fewer points.
getOrientationOfPointsreturns 1 if fewer than 3 points are found (line 145–147), but short shapes or degenerate cases might still appear. Consider explicitly catching these scenarios (e.g., a single-line path) or returning an error to make debugging easier.
168-184: Optional: Outline future stroke-related properties.
StrokeAttributesis comprehensive for basic stroke. If advanced features (e.g., gradients, dashes, or alpha) are planned, preemptively marking them as optional fields can keep the interface flexible without breaking changes later.
186-197: Prospective test coverage.
Before diving into the largebuildLinefunction, consider adding unit tests for each drawing scenario: open vs. closed shape, different caps and joins, zero-length segments, etc. This helps ensure correctness as the function has many branching paths.Would you like me to draft sample tests covering these scenarios?
packages/effects-core/src/components/shape-component.ts (2)
63-67: Consider caching the path value to prevent unnecessary rebuilds.The getter always sets
shapeDirty = truewhich could trigger unnecessary rebuilds even when the path hasn't changed.Consider caching the path value and only marking as dirty when it actually changes:
+private cachedPath: spec.CustomShapeData | null = null; get path () { - this.shapeDirty = true; + if (this.cachedPath !== this.data) { + this.shapeDirty = true; + this.cachedPath = this.data as spec.CustomShapeData; + } - return this.data as spec.CustomShapeData; + return this.cachedPath; }
116-122: Consider adding validation for stroke attributes.The stroke attributes initialization looks good, but consider adding validation to ensure the values meet the requirements.
Add validation for the stroke attributes:
this.strokeAttributes = { width: 1, alignment: 0.5, cap: 'butt', join: 'miter', miterLimit: 10, }; +this.validateStrokeAttributes(this.strokeAttributes); + +private validateStrokeAttributes(attrs: StrokeAttributes) { + if (attrs.width <= 0) { + throw new Error('Stroke width must be positive'); + } + if (attrs.alignment < 0 || attrs.alignment > 1) { + throw new Error('Stroke alignment must be between 0 and 1'); + } + if (attrs.miterLimit <= 0) { + throw new Error('Miter limit must be positive'); + } +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/effects-core/src/components/shape-component.ts(13 hunks)packages/effects-core/src/plugins/shape/build-line.ts(1 hunks)packages/effects-core/src/plugins/shape/poly-star.ts(1 hunks)packages/effects-core/src/plugins/shape/polygon.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/effects-core/src/plugins/shape/build-line.ts (1)
65-140: Validate angle calculations in theroundfunction.
The approach of usingatan2for arcs is valid, but the trigonometric usage is somewhat reversed (e.g.,Math.sin(angle)for x,Math.cos(angle)for y). This can be correct in certain coordinate systems, but double-check that it aligns with the rest of your geometry codebase and that you’re not introducing unexpected flips.packages/effects-core/src/plugins/shape/polygon.ts (1)
155-166: Validate offset usage intriangulate.
IntroducingindexStart = vertices.length / 2;and usingverticesOffset * 2 + ifor storing triangle coordinates is correct for XY-paired data. However, ensure that consumers expect this layout and aren’t interleaving other attributes in the same buffer. If so, you might need clearer documentation or adapt to a more flexible layout strategy.packages/effects-core/src/plugins/shape/poly-star.ts (1)
109-120: Consistent indexing strategy withindexStart.
This approach matches the recent changes inpolygon.ts, maintaining consistent offset application in triangulation. Just be sure the indexing pattern is compatible with other shape classes, especially if combined geometry buffering is used downstream.packages/effects-core/src/components/shape-component.ts (3)
13-14: LGTM! New stroke rendering support added.The new properties and imports for stroke support are well-structured and properly typed.
Also applies to: 28-28, 30-34
129-135: LGTM! Clean update logic.The update method correctly handles rebuilding the shape when dirty and resets the flag afterward.
151-158: Verify if all shapes should be closed when rendering strokes.The
closeparameter is hardcoded totrue, which might not be appropriate for all shape types.Consider making it configurable based on the shape type:
-const close = true; +const close = shapePrimitive.shape.shouldClose ?? true;
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/effects-core/src/plugins/timeline/playables/vector-property-mixer-playable.ts (2)
35-38: Use a more generic error message.
The error message references"Vector4PropertyTrack"even though this logic resides inVectorPropertyMixerPlayable. Consider updating it to reflect the current class name to avoid confusion.
55-69: Avoid duplicated property updates.
The logic for accumulating weighted values in bothVector4PropertyMixerPlayableandVector2PropertyMixerPlayableis similar, just handling different dimensions. You can refactor to reduce duplicate dimension-wise addition. For example, if your vector math library provides a method likeaddScaledVector(), you can leverage it:packages/effects-core/src/plugins/timeline/tracks/vector-property-track.ts (1)
25-40: Consider using a constant fromspec.DataTypefor the decorator.For consistency with
Vector4PropertyTrack, consider using a constant fromspec.DataTypeinstead of a string literal.Apply this diff to use a constant:
-@effectsClass('Vector2PropertyTrack') +@effectsClass(spec.DataType.Vector2PropertyTrack)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/effects-core/src/plugins/timeline/playables/index.ts(1 hunks)packages/effects-core/src/plugins/timeline/playables/vector-property-mixer-playable.ts(1 hunks)packages/effects-core/src/plugins/timeline/playables/vector4-property-mixer-playable.ts(0 hunks)packages/effects-core/src/plugins/timeline/tracks/index.ts(1 hunks)packages/effects-core/src/plugins/timeline/tracks/vector-property-track.ts(2 hunks)web-packages/test/case/src/2d/assets/inspire.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/effects-core/src/plugins/timeline/playables/vector4-property-mixer-playable.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/effects-core/src/plugins/timeline/playables/vector-property-mixer-playable.ts (1)
20-26: VerifysetZeroavailability.
Ensure that allVector2orVector4instances indeed implement thesetZero()method at runtime. If there's any situation where thevaluemight not be initialized or might not have this method, consider adding additional checks or fallback logic.packages/effects-core/src/plugins/timeline/tracks/index.ts (1)
9-9: Export update looks good.
Switching to./vector-property-trackis consistent with the new unified approach for vector types.packages/effects-core/src/plugins/timeline/playables/index.ts (1)
11-11: Exportingvector-property-mixer-playableis consistent with the new architecture.
This aligns with the introduction of the genericVectorPropertyMixerPlayablesupporting both Vector2 and Vector4.packages/effects-core/src/plugins/timeline/tracks/vector-property-track.ts (1)
8-23: LGTM!The implementation is correct and follows the expected pattern for property track creation and configuration.
web-packages/test/case/src/2d/assets/inspire.ts (1)
22-25: LGTM!The new effect entry follows the established pattern and maintains consistency with other entries.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/effects-core/src/plugins/timeline/playables/float-property-mixer-playable.ts (1)
4-6: Correct the method name to improve readability.
Same spelling issue here withresetAnimatedPropery(). Please fix it for consistency across the codebase.- override resetAnimatedPropery (): void { + override resetAnimatedProperty (): void {packages/effects-core/src/plugins/timeline/playables/property-mixer-playable.ts (2)
9-9: Fix typo in method name
RenameresetAnimatedProperytoresetAnimatedPropertyfor consistency and clarity.-abstract resetAnimatedPropery (): void; +abstract resetAnimatedProperty (): void;
34-34: Refine type casting flow
After casting toPropertyClipPlayable<T>, you still checkinstanceof PropertyClipPlayable. Consider a narrower type guard or adjusting the control flow to avoid unnecessary casting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts(1 hunks)packages/effects-core/src/plugins/timeline/playables/float-property-mixer-playable.ts(1 hunks)packages/effects-core/src/plugins/timeline/playables/property-mixer-playable.ts(2 hunks)packages/effects-core/src/plugins/timeline/playables/vector-property-mixer-playable.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects-core/src/plugins/timeline/playables/vector-property-mixer-playable.ts
🧰 Additional context used
🧠 Learnings (1)
packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts (1)
Learnt from: wumaolinmaoan
PR: galacean/effects-runtime#691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-11-12T14:51:03.288Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts (2)
1-2: Imports look good!
No issues found in these import statements.
9-15: Good approach for color accumulation without cloning.
Incrementally adding each color component matches the recommended practice to avoid the additional GC overhead of cloning. Keep verifying thatthis.propertyValueis not shared elsewhere, so its cumulative updates remain correct.packages/effects-core/src/plugins/timeline/playables/float-property-mixer-playable.ts (2)
1-1: No concerns about the import.
The import ofPropertyMixerPlayableis straightforward and aligns with the new class design.
8-9: Simple and efficient weighted addition.
AddingcurveValue * weightto the property is succinct and avoids unnecessary calls or intermediate objects.packages/effects-core/src/plugins/timeline/playables/property-mixer-playable.ts (9)
5-5: Good use of an abstract generic class
IntroducingPropertyMixerPlayable<T>as an abstract class effectively supports multiple property types and encourages extensibility. This design appears well-structured.
7-7: Consider initializingpropertyValue
propertyValueis declared but never assigned a default. Ifevaluateis never called, the property may remain uninitialized. Confirm that no other code path uses it prematurely.
10-10: Method signature looks fine
This abstract method is well-scoped to handle weighted additions for any generic typeT.
21-21: Verify cast correctness
CastingboundObject[this.propertyName]toTcould mask incompatible runtime types. Consider a type check or fallback handling for safer usage.
23-23: Null/undefined check
The early return onnullorundefinedmay skip necessary logic. Confirm this behavior aligns with your design requirements.
27-27: Duplicate comment: spelling
This line also callsresetAnimatedPropery, which repeats the misspelling noted earlier.
37-37: Consider throwing an error
Logging a console error may not adequately handle a mismatch with non-PropertyPlayableAssetobjects. Evaluate whether throwing an error is more appropriate to fail fast in production scenarios.
43-43: Proper abstraction usage
this.addWeightedValueapplies well here, leveraging the new abstract method for typed accumulation.
51-51: Consistent assignment
Assigning the mixedpropertyValueback toboundObjectaligns with the intended design, ensuring updated values are written just once.
packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/effects-core/src/plugins/timeline/playables/property-mixer-playable.ts (2)
21-25: Consider adding type guard for property value.The type cast to T might fail silently. Consider adding a type guard to ensure type safety.
- this.propertyValue = boundObject[this.propertyName] as T; - - if (this.propertyValue === undefined || this.propertyValue === null) { + const value = boundObject[this.propertyName]; + + if (!this.isValidPropertyValue(value)) { return; } + this.propertyValue = value as T;Add this protected method to the class:
protected abstract isValidPropertyValue(value: unknown): value is T;
34-39: Improve error handling for invalid playable assets.The error message could be more descriptive to help with debugging.
- console.error('PropertyTrack added non-PropertyPlayableAsset'); + console.error(`PropertyTrack expected PropertyClipPlayable but got ${propertyClipPlayable.constructor.name}`);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts(1 hunks)packages/effects-core/src/plugins/timeline/playables/float-property-mixer-playable.ts(1 hunks)packages/effects-core/src/plugins/timeline/playables/property-mixer-playable.ts(2 hunks)packages/effects-core/src/plugins/timeline/playables/vector-property-mixer-playable.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects-core/src/plugins/timeline/playables/vector-property-mixer-playable.ts
🧰 Additional context used
🧠 Learnings (1)
packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts (1)
Learnt from: wumaolinmaoan
PR: galacean/effects-runtime#691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-11-12T14:51:03.288Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
🔇 Additional comments (3)
packages/effects-core/src/plugins/timeline/playables/property-mixer-playable.ts (1)
5-7: LGTM! Good use of generics for type-safe property mixing.The abstract class with generic type parameter T provides a flexible and type-safe base for different property types.
packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts (1)
9-16: LGTM! Efficient color mixing implementation.The implementation directly modifies color components without creating new objects, which aligns with the previous learning about avoiding
clone()for GC optimization.packages/effects-core/src/plugins/timeline/playables/float-property-mixer-playable.ts (1)
3-11: LGTM! Clean and efficient implementation for float mixing.The implementation is straightforward and efficient, using simple arithmetic operations for float value mixing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/effects-core/src/plugins/shape/build-line.ts (1)
194-546: 🛠️ Refactor suggestionExtract complex logic into smaller functions.
The
buildLinefunction is over 300 lines long and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions for better maintainability and testability.Here's a suggested breakdown:
calculateLinePoints: Handle point calculations and transformationshandleLineStart: Handle the start of the linehandleLineSegment: Process each line segmenthandleLineEnd: Handle the end of the linegenerateIndices: Generate triangle indicesExample refactor for the line start:
function handleLineStart( x0: number, y0: number, perpX: number, perpY: number, style: StrokeAttributes, innerWeight: number, outerWeight: number, verts: number[] ): number { let indexCount = 0; if (style.cap === 'round') { indexCount += round( x0 - (perpX * (innerWeight - outerWeight) * 0.5), y0 - (perpY * (innerWeight - outerWeight) * 0.5), x0 - (perpX * innerWeight), y0 - (perpY * innerWeight), x0 + (perpX * outerWeight), y0 + (perpY * outerWeight), verts, true, ) + 2; } else if (style.cap === 'square') { indexCount += square(x0, y0, perpX, perpY, innerWeight, outerWeight, true, verts); } return indexCount; }
🧹 Nitpick comments (4)
packages/effects-core/src/plugins/shape/build-line.ts (4)
6-7: Consider using more descriptive constant names.The constants
closePointEpsandcurveEpscould benefit from more descriptive names that better explain their purpose, such asPOINT_DISTANCE_THRESHOLDandCURVE_AREA_THRESHOLD.-export const closePointEps = 1e-4; -export const curveEps = 0.0001; +export const POINT_DISTANCE_THRESHOLD = 1e-4; +export const CURVE_AREA_THRESHOLD = 0.0001;
24-62: Consider adding input validation.The
squarefunction should validate its input parameters, especially for potential NaN or infinite values in coordinates and weights.function square ( x: number, y: number, nx: number, ny: number, innerWeight: number, outerWeight: number, clockwise: boolean, verts: Array<number> ): number { + if (!Number.isFinite(x) || !Number.isFinite(y) || + !Number.isFinite(nx) || !Number.isFinite(ny) || + !Number.isFinite(innerWeight) || !Number.isFinite(outerWeight)) { + throw new Error('Invalid coordinates or weights'); + } + if (!Array.isArray(verts)) { + throw new Error('Invalid vertex buffer'); + } const ix = x - (nx * innerWeight); const iy = y - (ny * innerWeight);
539-542: Consider using a more descriptive variable name for clarity.The variable
eps2could be renamed to better explain its purpose in the area calculation.-if (Math.abs((x0 * (y1 - y2)) + (x1 * (y2 - y0)) + (x2 * (y0 - y1))) < eps2) { +const MIN_TRIANGLE_AREA = eps2; +if (Math.abs((x0 * (y1 - y2)) + (x1 * (y2 - y0)) + (x2 * (y0 - y1))) < MIN_TRIANGLE_AREA) {
357-386: Add a comment explaining the parallel line handling logic.The parallel line detection and handling logic could benefit from a more detailed explanation of the mathematical approach used.
- /* Going nearly parallel? */ - /* atan(0.001) ~= 0.001 rad ~= 0.057 degree */ + /* Check if lines are nearly parallel by comparing cross product to dot product + * If cross/dot < 0.001 (atan(0.001) ≈ 0.057°), lines are considered parallel + * This threshold provides a good balance between accuracy and numerical stability + */ if (Math.abs(cross) < 0.001 * Math.abs(dot)) {
…ects-runtime into feat/multi-shape-and-stroke
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/effects-core/src/plugins/shape/triangle.ts (2)
183-208: Consider improving readability of the triangulate method.The triangulate method could be more readable with named constants and clearer structure.
Consider this refactor:
override triangulate (points: number[], vertices: number[], verticesOffset: number, indices: number[], indicesOffset: number): void { - let count = 0; const verticesStride = 2; + const vertexCount = 3; // triangle has 3 vertices + const coordsPerVertex = 2; // x and y coordinates verticesOffset *= verticesStride; - vertices[verticesOffset + count] = points[0]; - vertices[verticesOffset + count + 1] = points[1]; - - count += verticesStride; - - vertices[verticesOffset + count] = points[2]; - vertices[verticesOffset + count + 1] = points[3]; - - count += verticesStride; - - vertices[verticesOffset + count] = points[4]; - vertices[verticesOffset + count + 1] = points[5]; + // Copy vertices + for (let i = 0; i < vertexCount; i++) { + const vertexOffset = verticesOffset + (i * coordsPerVertex); + const pointOffset = i * coordsPerVertex; + vertices[vertexOffset] = points[pointOffset]; + vertices[vertexOffset + 1] = points[pointOffset + 1]; + } const verticesIndex = verticesOffset / verticesStride; - // triangle 1 + // Add indices for the single triangle indices[indicesOffset++] = verticesIndex; indices[indicesOffset++] = verticesIndex + 1; indices[indicesOffset++] = verticesIndex + 2; }
86-99: Clarify plans for the commented-out strokeContains method.The commented-out
strokeContainsmethod suggests incomplete stroke support. Is this planned for future implementation?Would you like me to help implement the stroke containment check? The current implementation seems to reference an undefined
squaredDistanceToLineSegmentfunction.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/effects-core/NOTICE(1 hunks)packages/effects-core/src/plugins/shape/build-line.ts(1 hunks)packages/effects-core/src/plugins/shape/ellipse.ts(1 hunks)packages/effects-core/src/plugins/shape/graphics-path.ts(1 hunks)packages/effects-core/src/plugins/shape/point-data.ts(1 hunks)packages/effects-core/src/plugins/shape/point-like.ts(1 hunks)packages/effects-core/src/plugins/shape/point.ts(1 hunks)packages/effects-core/src/plugins/shape/poly-star.ts(2 hunks)packages/effects-core/src/plugins/shape/polygon.ts(2 hunks)packages/effects-core/src/plugins/shape/rectangle.ts(1 hunks)packages/effects-core/src/plugins/shape/shape-path.ts(1 hunks)packages/effects-core/src/plugins/shape/shape-primitive.ts(1 hunks)packages/effects-core/src/plugins/shape/triangle.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/effects-core/src/plugins/shape/shape-primitive.ts
- packages/effects-core/src/plugins/shape/point.ts
- packages/effects-core/src/plugins/shape/point-data.ts
- packages/effects-core/src/plugins/shape/ellipse.ts
- packages/effects-core/src/plugins/shape/shape-path.ts
- packages/effects-core/src/plugins/shape/rectangle.ts
- packages/effects-core/src/plugins/shape/graphics-path.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/effects-core/src/plugins/shape/poly-star.ts
- packages/effects-core/src/plugins/shape/polygon.ts
- packages/effects-core/src/plugins/shape/build-line.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/effects-core/src/plugins/shape/point-like.ts (1)
1-35: Well-designed interface with clear method signatures!The
PointLikeinterface is well-documented and follows good practices:
- Clear method signatures with proper return types
- Generic type parameter in
copyToensures type safety- Flexible
setmethod with optional parameters- Thorough JSDoc comments
packages/effects-core/src/plugins/shape/triangle.ts (1)
1-209: Solid implementation with efficient algorithms!The Triangle class is well-implemented with proper coordinate handling and efficient algorithms for point containment and triangulation.
Summary by CodeRabbit
Triangleshape class for geometric representation.ColorPropertyMixerPlayableandFloatPropertyMixerPlayablefor direct property manipulation.PolygonandPolyStarclasses for better data handling.