-
Notifications
You must be signed in to change notification settings - Fork 52
[HLSL] define the matrix element accessor semantics #678
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
base: main
Are you sure you want to change the base?
Conversation
148e920 to
f02f52f
Compare
llvm-beanz
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.
A few suggestions, but otherwise LGTM.
Co-authored-by: Chris B <[email protected]>
Co-authored-by: Chris B <[email protected]>
Co-authored-by: Chris B <[email protected]>
Co-authored-by: Chris B <[email protected]>
V-FEXrt
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.
Approving because I'll be out next week. I'm happy with the change as long as the _m1_m2 issue is resolved
|
|
||
| \define{matrix-one-indexed-swizzle-sequence}\br | ||
| matrix-one-indexed-swizzle\br | ||
| matrix-one-indexed-swizzle-sequence matrix-one-indexed-swizzle\br |
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'm not exactly sure the best way to rewrite this, but as written this would require _m1_m2 instead of _m12 right?
| matrix-one-indexed-swizzle-sequence matrix-one-indexed-swizzle\br | ||
|
|
||
| \define{matrix-one-indexed-swizzle}\br | ||
| \terminal{\_m} one-index-value\br |
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.
| \terminal{\_m} one-index-value\br | |
| \terminal{\_} one-index-value\br |
Should be just _ not _m for 1-indexing right?
|
|
||
| \define{matrix-zero-indexed-swizzle-sequence}\br | ||
| matrix-zero-indexed-swizzle\br | ||
| matrix-zero-indexed-swizzle-sequence matrix-zero-indexed-swizzle\br |
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'm not exactly sure the best way to rewrite this, but as written this would require _m1_m2 instead of _m12 right?
| \end{itemize} | ||
|
|
||
| \p A \textit{matrix-swizzle-component-sequence} is a sequence of one but less | ||
| than or equal to four swizzle components of either the |
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.
imo it would be better to encode this into the grammar
its not too crazy to write
matrix-one-indexed-swizzle-sequence:
"_m", zero-index-value
| "_m", zero-index-value, zero-index-value
| "_m", zero-index-value, zero-index-value, zero-index-value
| "_m", zero-index-value, zero-index-value, zero-index-value, zero-index-value
which also solves the _m1_m2 issue mentioned above
fixes #677
Moved the Vector Swizzle stuff that repeats with Matrix and moved it to a generic swizzle section.

The new Matrix Swizzle looks like this:
