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

Implement AnyValue marshalling for 8-bit integers #6059

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

juliusikkala
Copy link
Contributor

@juliusikkala juliusikkala commented Jan 10, 2025

I think this closes #4817?

Based on the discussion in that issue, I implemented this using the now-existing BitfieldInsert & BitfieldExtract instead of the shift and mask approach (which is currently used for 16-bit types) and added a test.

I only realized that there was already an issue about this after having already implemented the shift + mask version, and I have to say that this BitfieldInsert & Extract approach is much cleaner to implement as well.

@juliusikkala juliusikkala requested a review from a team as a code owner January 10, 2025 18:40
csyonghe
csyonghe previously approved these changes Jan 10, 2025
Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

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

I didn't realize that this is missing. Thank you for completing the implementation!

@csyonghe csyonghe added the pr: non-breaking PRs without breaking changes label Jan 10, 2025
@juliusikkala
Copy link
Contributor Author

juliusikkala commented Jan 10, 2025

Whoops, missed that _getAnyValueSizeRaw actually tracks offset, not exactly size of the individual parameter. Hopefully that's what caused that Windows DX12 test to fail.

csyonghe
csyonghe previously approved these changes Jan 10, 2025
@juliusikkala
Copy link
Contributor Author

juliusikkala commented Jan 11, 2025

I took out that DXIL test. I figured out that it's not possible to compile code containing uint8_t there, this is in the same boat as #3591. Which is why that test fails too. This feature has been backlogged for a future shader model on Microsoft's side, so there's not much we can do for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
2 participants