Open
Conversation
📊 Bundle Size Comparison
👀 Notable resultsStatic test results:No major changes. Dynamic test results:No major changes. 📋 All resultsClick to reveal the results table (344 entries).
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu. |
|
pkg.pr.new packages benchmark commit |
sharabai
force-pushed
the
feat/rotating-triangle-tiles-example
branch
from
f658ee6 to
5086460
Compare
sharabai
force-pushed
the
feat/rotating-triangle-tiles-example
branch
from
7b8d09b to
2ce6e6c
Compare
Contributor
aleksanderkatan
left a comment
aleksanderkatan
left a comment
There was a problem hiding this comment.
Great first example! 🦋
I tried to be educational in the comments, so many of them are not necessarily issues but stylistic preferences, nits or good practices.
If you have no time to address these comments, we can take over, no problem
| @@ -0,0 +1,49 @@ | |||
| // Generate by AI | |||
Contributor
There was a problem hiding this comment.
Could you find some human made implementation and reference the author instead? Or, if it's just some wikipedia formulas, just remove this line
Collaborator
Author
There was a problem hiding this comment.
@aleksanderkatan maybe it was better to leave this as a comment in github PR.
Just wanted to tell people not to ask me about the math here.
I'm pretty sure this is generic, foundational math.
Comment on lines
+14
to
+16
| const shiftedColorsBuffer = root.createReadonly(d.arrayOf(d.vec4f, 3), [ | ||
| ...colors, | ||
| ]); |
Contributor
There was a problem hiding this comment.
Suggested change
| const shiftedColorsBuffer = root.createReadonly(d.arrayOf(d.vec4f, 3), [ | |
| ...colors, | |
| ]); | |
| const shiftedColorsBuffer = root.createReadonly(d.arrayOf(d.vec4f, 3), colors); |
Comment on lines
+4
to
+5
| "tags": ["experimental", "fragment shader", "instancing"], | ||
| "dev": false |
Contributor
There was a problem hiding this comment.
Suggested change
| "tags": ["experimental", "fragment shader", "instancing"], | |
| "dev": false | |
| "tags": ["experimental", "fragment shader", "instancing"] |
Collaborator
Author
There was a problem hiding this comment.
@aleksanderkatan makes sense since it's a default value
Comment on lines
+17
to
+19
| function getCubicBezierControlPoints() { | ||
| return cubicBezierControlPoints; | ||
| } |
Contributor
There was a problem hiding this comment.
Can this function be inlined?
Collaborator
Author
There was a problem hiding this comment.
@aleksanderkatan could be, but I just didn't finish one thing it the example. Right now, if you click on Edit Cubic Bezier Points it would open up with those hardcoded values. But now, after adding updating logic, this getCubicBezierControsPoints function makes sense because it gives different outputs.
Comment on lines
+91
to
+96
| const GridParams = d.struct({ | ||
| tileDensity: d.f32, | ||
| userScale: d.f32, | ||
| trianglesPerRow: d.u32, | ||
| triangleCount: d.u32, | ||
| }); |
Contributor
There was a problem hiding this comment.
I think this struct is unused
Comment on lines
+84
to
+85
| let calculatedPosition = rotate(vertexPosition, angle); | ||
| calculatedPosition = std.mul(calculatedPosition, scaleFactor); |
Contributor
There was a problem hiding this comment.
nit: we support infix operators for +-*/
Suggested change
| let calculatedPosition = rotate(vertexPosition, angle); | |
| calculatedPosition = std.mul(calculatedPosition, scaleFactor); | |
| const calculatedPosition = rotate(vertexPosition, angle).mul(scaleFactor); |
Comment on lines
+136
to
+138
| if ((e0 > 0 || e1 > 0 || e2 > 0) && drawOverNeighborsAccess.$ === 0) { | ||
| std.discard(); | ||
| } |
Contributor
There was a problem hiding this comment.
This drawOverNeighbors switch has made this shader a lot more complicated. Without this, we could probably simplify everything to only one straightforward pipeline.
I tried to come up with something to avoid passing so much flat interpolated data and without discard. We could theoretically create a static stencil like the following:
And use it for "odd" triangles, use a negative stencil for "even" triangles, and no stencil for the front layer. Maybe @reczkok can come up with something better.
Collaborator
Author
There was a problem hiding this comment.
@aleksanderkatan
I had working stencils at Add instances commit.
rotating.triangles.mp4
The problem was that with instances always drew over each other.
Comment on lines
+11
to
+17
| const stepRotationAccess = tgpu['~unstable'].accessor(d.f32); | ||
| const shiftedColorsAccess = tgpu['~unstable'].accessor(d.arrayOf(d.vec4f, 3)); | ||
| const animationProgressAccess = tgpu['~unstable'].accessor(d.f32); | ||
| const middleSquareScaleAccess = tgpu['~unstable'].accessor(d.f32); | ||
| const drawOverNeighborsAccess = tgpu['~unstable'].accessor(d.u32); | ||
| const scaleAccess = tgpu['~unstable'].accessor(d.f32); | ||
| const aspectRatioAccess = tgpu['~unstable'].accessor(d.f32); |
Contributor
There was a problem hiding this comment.
tip: There is a pretty small limit of buffers that can be used without bind groups, usually it's only 12 (because this is the minimum limit for a single bind group, and those are bound under the hood). A simple way to avoid it is to group those into structs. In this case, it is not necessary, though it might clean up the code a little bit.
Collaborator
Author
There was a problem hiding this comment.
@aleksanderkatan got it, I'd keep in mind.
| aspectRatioBuffer.write(aspectRatio); | ||
| } | ||
|
|
||
| let animationDuration = 1000; |
Contributor
There was a problem hiding this comment.
This variable is kept between example switches (triangles -> sth else -> triangles). It's just a number so this probably does not matter.
Collaborator
Author
There was a problem hiding this comment.
Same for cubicBezierControlPoints, although I don't see any harm.
| // Example controls | ||
|
|
||
| export const controls = { | ||
| 'Tile density': { |
Contributor
There was a problem hiding this comment.
In examples we usually start controls' names with lowercase letters (exception: buttons)
Collaborator
Author
There was a problem hiding this comment.
@aleksanderkatan
I don't know about that. I've seen many where capitalized names are the norm. Like jump-flood-voronoi, jump-flood-distance, jelly-slider to name a few.
Imo, it's better to handle it at the level of Controls themselves at this point. Just make the names there capitalized or not if consistency in control names is important. At this point, it's a mix of both. Hard to say which one is a standard.
sharabai
force-pushed
the
feat/rotating-triangle-tiles-example
branch
from
d2c2ee4 to
230ee77
Compare
sharabai
force-pushed
the
feat/rotating-triangle-tiles-example
branch
from
230ee77 to
290b461
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Triangle tiles that rotate.