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

Examples: Added webgpu_multiple_rendertargets_readback #28211

Merged
merged 19 commits into from
Apr 25, 2024
Merged

Conversation

Spiri0
Copy link
Contributor

@Spiri0 Spiri0 commented Apr 24, 2024

This is a new example that demonstrates the use of multiple render targets in conjunction with the readRenderTargetPixelsAsync. The readRenderTargetPixelsAsync function was extended by an index with PR #28197 in order to be able to read any texture from a multiple renderTarget

Related issue: #28197

Spiri0 and others added 2 commits April 24, 2024 13:55
I removed the .html at the end of "webgpu_mrt_readback.html",
now:
"webgpu_mrt_readback",
@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 25, 2024

I'll improve the example with time. Although it works exactly as it should, an example should also be as efficient as possible in order to convey an efficient programming style. I would also like to add a size selection so that you can see different readback resolutions.

@sunag
Copy link
Collaborator

sunag commented Apr 25, 2024

Seems that readback normals is not working.
image

@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 25, 2024

Interesting, it worked for me and aardgoose.

3 (5)

Do you have an error message? I'll take a look at it right away

@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 25, 2024

Oh, has the extension of the readRenderTargetPixelsAsync already been taken into account where you are testing it? This doesn't work without #28197

better code. Previously, new instances were constantly being created, which was unnecessary and inefficient.
if(selector == 2){
const pixelBuffer0 = await renderer.readRenderTargetPixelsAsync(readbackTarget, 0, 0, width, height, 0); //zero is optional

const pixelBufferTexture0 = new THREE.DataTexture(pixelBuffer0, width, height);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems that DataTexture and Material are created in each frame too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, something else slipped through my fingers. Yes, of course that shouldn't be the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it looking good. Have you meanwhile seen the readback_Normal with taking #28197 into account?

@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 25, 2024

I took the opportunity to make the example a little better. I then deactivated the #28197 extension and then the readback_Normal didn't work for me either. So the same result as yours. That will definitely be the cause for you too.

Now no new instances are created during the update, only data is updated as it should be
Now no new instances are created during the update, only data is updated as it should be
Now no new instances are created during the update, only data is updated as it should be
@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 25, 2024

The normal readback does not work for me as well. Please make sure to rebase the branch so it is based on latest dev. You should not expect from reviewers to copy over changes from other branches.

This branch must be fully functional so it can be properly reviewed.

const width = readbackTarget.width;
const height = readbackTarget.height;

if(selector == 2){
Copy link
Collaborator

Choose a reason for hiding this comment

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

The formatting of the code is still not correct. Please make sure ESLint properly lints the code according to the project's code style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My apologies, I thought I did that with the rebase. This is still new for me and I admit there is still a lot I don't know about github.
By formatting do you mean
if( selector == 2 ) {
instead of
if(selector == 2){
?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. And ideally use === for strict comparison.

Don't worry if you need time to get things going.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your understanding. I can easily solve the formatting issue. About the rebase, is there anyone I can ask how I do it correctly?

Copy link
Collaborator

@Mugen87 Mugen87 Apr 25, 2024

Choose a reason for hiding this comment

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

You can use the merge or rebase command. Normally you start by fetching the latest commit from the upstream repository:

git fetch upstream

You can then merge the latest changes on dev into your branch:

git merge upstream/dev

Or perform a rebase:

git rebase upstream/dev

The are many resources online which explain the difference between merge and rebase. Both have their pros and cons. However, rebase is ideal for situations when you want to move the point where you have branched to a new starting point. This is exactly what you are looking for in this instance.

Spiri0 and others added 7 commits April 25, 2024 12:39
corrected formatting and strict comparison
corrected formatting and strict comparison
corrected formatting and strict comparison
corrected formatting and strict comparison
@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 25, 2024

I won't get back to it until tonight as I'm at work.

@sunag
Copy link
Collaborator

sunag commented Apr 25, 2024

I won't get back to it until tonight as I'm at work.

I made some revisions, I think it's ok now. Thank you for the example and fixes :)

@sunag sunag changed the title Add new example: webgpu_mrt_readback.html and update files.json Apr 25, 2024
@sunag sunag merged commit 2182dc3 into mrdoob:dev Apr 25, 2024
11 checks passed
@Mugen87 Mugen87 added this to the r164 milestone Apr 25, 2024
@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 25, 2024

Oh, merged? I haven't done the rebase yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants