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

Expand resource copying APIs #161

Merged
merged 19 commits into from
Nov 14, 2021
Merged

Conversation

Sergio0694
Copy link
Owner

Closes #158

This PR includes additions and improvements to all resource copying APIs, as detailed in the linked issue.

cc. @hawkerm for review 😄

@Sergio0694 Sergio0694 added the enhancement ✨ An improvement to existing APIs label Nov 12, 2021
Copy link
Contributor

@hawkerm hawkerm left a comment

Choose a reason for hiding this comment

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

Going to see if I can test out with the sample I was building, went though with some questions/comments though for you. Thanks! 😊

@hawkerm
Copy link
Contributor

hawkerm commented Nov 13, 2021

These were the weird failing tests right? Known to fail sometimes locally on certain machines?

image

image

@hawkerm
Copy link
Contributor

hawkerm commented Nov 13, 2021

Still had to modify the IShaderRunner interface, but think that was expected still (but worked to copy!):

public void Execute(ReadWriteTexture2D<Rgba32, Float4> texture, TimeSpan timespan);

@Sergio0694
Copy link
Owner Author

@hawkerm That is correct, those tests are somewhat bugged, but only on some machines and only under very specific cases. I'll see if I can get a repro together this weeked so that I can ping the D3D12MA engineer as well as the WARP team if needed 😄

And yes, changing that input texture is something we can look into in another PR.
What was your use case again for needing to copy it?

@hawkerm
Copy link
Contributor

hawkerm commented Nov 14, 2021

And yes, changing that input texture is something we can look into in another PR. What was your use case again for needing to copy it?

For #137 to be able to grab a copy of the result texture to pass in as input to the next run of the shader. As the texture passed in is empty each loop through right?

Realizing though that if I just have the buffer texture array anyway I can just do the calculations beforehand from that and store it in itself and may be the same... Was just thinking it may make math easier. I'll play around a bit more...

@Sergio0694
Copy link
Owner Author

"For #137 to be able to grab a copy of the result texture to pass in as input to the next run of the shader."

Ah, right. Yeah in that case you'd need a temporary buffer every frame otherwise you'd have shader invocations do side effect on the other pixels that are being read by other invocations (if every invocation is not just reading a single pixel at that corresponding location, but a window of pixels or any other group of pixels in general), and things would get messed up.

"As the texture passed in is empty each loop through right?"

No, the input texture will always have the data from the previous frame. So if that's enough for you and you're just having each pixel read on the corresponding pixel from the previous frame, you don't really need the additional temporary buffer.

@Sergio0694 Sergio0694 merged commit 05407c0 into main Nov 14, 2021
@Sergio0694 Sergio0694 deleted the feature/resource-copy-apis-revamp branch November 14, 2021 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ An improvement to existing APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve and streamline resource copying APIs
2 participants