-
-
Notifications
You must be signed in to change notification settings - Fork 45
feat: Add all numeric and derivative std (some with no cpu implementation) #1567
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
Conversation
ec41eb0 to
a2bc9f6
Compare
|
pkg.pr.new packages benchmark commit |
iwoplaza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeppers!
lursz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked for types mismatch against wgsl spec, everything checks out!
Great work
93356f3 to
6aea8ec
Compare
aleksanderkatan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! 🐥
packages/typegpu/src/data/struct.ts
Outdated
| [$internal]: { | ||
| builtinName: undefined, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we make this property optional and just put {} here? Could we make {} a default instead of true for internals in general, or is it too much overhead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like the verbosity in this particular case - but now that I think about it it should be null instead of undefined. Also I don't see a reason to change it to {} at least for this particular pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the functions are missing the private remarks. I wonder, are they even necessary? I would either opt to remove them entirely, or to make them regular jsdocs so that users can easily check which functions are WGSL builtins and which are not (allEq, operators, isCloseTo, potentially ulog2 in the future etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think turning them into regular jscocs is a good idea
iwoplaza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's gooooo
Co-authored-by: Iwo Plaza <iwoplaza@gmail.com>
1d1284b to
cc5d9cd
Compare
I also added a
builtin structfor internal use. These represent return types for some std functions. They do not add definitions and do not alter their names.