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

Add compute texture demo #938

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Jul 24, 2023

This PR adds a demo that shows off using a compute shader to populate a texture.

Note this requires godotengine/godot#79288 and godotengine/godot#79696, both are merged into master.

image

@Lateasusual
Copy link

Most times that I run the demo the simulation breaks due to the texture data being full of NaNs, I think the textures should be cleared at the start.

@BastiaanOlij
Copy link
Contributor Author

Most times that I run the demo the simulation breaks due to the texture data being full of NaNs, I think the textures should be cleared at the start.

Hmm, my graphics card driver must do that automatically. Easy fix though, try it again :)

@GeorgeS2019
Copy link

@BastiaanOlij
@Calinou

Consider using this as demo for godot 4 Compute Shader

msedge_DL7Q36CGJ9

image

#Source
Youtube

Code:

currently 80% works with Godot_v4.0-beta2_win64

@BastiaanOlij
Copy link
Contributor Author

@GeorgeS2019 That shows a different use case of compute shaders than what we're showcasing here, definately worth having a demo like that here.

That should be a question asked to the author of that demo, whether he'd like to add a PR here to submit (a version of) his project. It would be rude to just add it.

@GeorgeS2019
Copy link

Already asked 😉

@henriksod
Copy link

henriksod commented Jul 29, 2023

Hi! I am the author of that Godot project. I have no problem with this being added to the list of official demo projects 😄

I think it needs some work though, will definitely look into how it can qualify as a demo project here.

I remember that for this project I had struggles with the lack of multi-pass post processing in the engine. I suppose this has not been implemented yet according to godotengine/godot-proposals#496

@dotlogix
Copy link

dotlogix commented Jul 30, 2023

@BastiaanOlij I quickly ported your code 1:1 to C# any interest adding this to the demo as well?
TestCustomTextures.zip

Thx a lot btw for implementing this this opens so many doors now for high-performance apps in Godot

Much Love <3

@GeorgeS2019
Copy link

@dotlogix

Thanks for your contribution.

We are getting many users from Unity3D, contributing Godot4 c# DEMO is ONE significant way to welcome them.

Thanks again

@BastiaanOlij
Copy link
Contributor Author

@henriksod cheers, I'll find some time to put a PR up. Watching your video I definately had some ideas for improvements so I might see what I can do, no promises :)

We are looking into adding something for post processing, but there is still a lot of debate on the direction that will take.

@BastiaanOlij
Copy link
Contributor Author

@dotlogix it'll be good to have C# versions of demos for those who want to use C#, but I'm not sure what our approach here is to include them.

@Calinou maybe you have some pointers here?

@Calinou
Copy link
Member

Calinou commented Jul 30, 2023

@Calinou maybe you have some pointers here?

The policy for C# demos is to avoid having C# ports of demos unless the scripting part is what they are demonstrating. This is because maintaining separate C# demos is a lot of work, so we try to keep the maintenance effort for them minimal.

@BastiaanOlij
Copy link
Contributor Author

btw @Calinou I'm happy for us to merge this as is, I'll revisit this when #79696 gets merged, that isn't a showstopping issue.

@Calinou
Copy link
Member

Calinou commented Jul 31, 2023

I'll revisit this when #79696 gets merged, that isn't a showstopping issue.

That PR was just merged 🙂

Could you look into revisiting this PR accordingly? I can merge this afterwards.

@BastiaanOlij
Copy link
Contributor Author

@Calinou will do!

@BastiaanOlij
Copy link
Contributor Author

Ok, I've added the multi-threaded protection code made possible by #79696 , and enabled multi-threading. It works nicely.

I did find one issue with assigning the texture_rd_rid on our Texture2DRD resource, as this assign needs to happen on the main thread, but the RD texture is not created until the rendering thread gets a turn, its not yet available on the first frame.

There is a sync issue left to solve here as we may not do our swap in our rendering thread before we run our process code but in worse case it runs a frame behind.
Talking to @reduz about how we should solve this issue.

@BastiaanOlij
Copy link
Contributor Author

Ok, I've solved the round robin issue, the only issue left now is that in multi-threaded mode, the very first frame our textures haven't been created yet.

We can create the texture objects from the main thread, and then do the rest of the work in the render thread, but it may send the wrong message to people looking at the code.
While RenderingDevice is thread safe, there are a lot of commands that will cause problems when called from the main thread.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested on 4.2.dev4, it works as expected. I'd wait until 4.2.stable is released to merge though.

There is a small visual issue though: the water's reflection appears to rotate incorrectly when you rotate the camera in the 3D editor viewport.

Also, please move the compute/texture/ folder to misc/compute_texture/, as it doesn't belong at the top-level. We try to keep the number of top-level folders to a minimum.

PS: We'll need to fix godotengine/godot#75659, as this project triggers it since it needs Multi-Threaded rendering model to work 🙂

@Oman395
Copy link

Oman395 commented Sep 15, 2023

I just spent several hours painfully working out how to actual use textures with godot compute shaders... I wish I'd seen this PR! One note-- it might help for there to be two demos, one that's simpler and one that's more complex, to help with the fundamentals without cluttering it with logic that's not 100% related just to using textures with compute-- maybe something as simple as a black screen that brightens, by adding onto the color of the previous frame each frame, using imageStore rather than just making it a fragment shader or sum to help teach how to do random writes, or maybe even just a shader that writes the uv texture onto an input image (this is based on the things I has to figure out manually so I could actually use the shaders in the way I want to...)

@BastiaanOlij
Copy link
Contributor Author

@Calinou the reason I created the compute folder is that I want to make a number of different examples that all use compute shaders. People will be looking specifically for these types of examples so I wonder if Misc is a good place.

@BastiaanOlij
Copy link
Contributor Author

@Oman395 definately planning on doing more examples :)

@BastiaanOlij
Copy link
Contributor Author

Figured out the dumb mistake I made in the normal calculation, logic I was using was the calculation you normally use in a vertex shader. So just adjusted this to work by mimicking a normal map.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Should be good to merge after applying suggestions.

compute/texture/main.gd Outdated Show resolved Hide resolved
compute/texture/water_plane/water_compute.glsl Outdated Show resolved Hide resolved
compute/texture/water_plane/water_compute.glsl Outdated Show resolved Hide resolved
compute/texture/water_plane/water_compute.glsl Outdated Show resolved Hide resolved
compute/texture/water_plane/water_compute.glsl Outdated Show resolved Hide resolved
compute/texture/water_plane/water_plane.gd Outdated Show resolved Hide resolved
compute/texture/water_plane/water_plane.gd Outdated Show resolved Hide resolved
# Even though we're using 3 sets, they are identical, so we're kinda cheating
return rd.uniform_set_create([uniform], shader, 0)

func _initialize_compute_code(p_texture_size):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func _initialize_compute_code(p_texture_size):
func _initialize_compute_code(texture_size):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change these if that is the style we're now going for, but I personally dislike parameters that look like normal variables, there is a higher chance of making mistakes or accidentally overriding member variables and introducing bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And indeed, this turns out to be the case for nearly every parameter. I'll see what I can do..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, in this case I renamed it to init_with_texture_size, bit lengthy but now it no longer clashes with the member variable.

It's a little paranoid but the reason this parameter shadows the member variable is because this code is designed on a different thread. It's possible that once the code runs on the render thread, the underlying member is changed on the main thread.

It's not really designed to work that way, we would need to do more if someone changes the texture size after the node is added to the tree, but it is good practice to be consistent in not simply accessing member variables in threads.

Copy link
Member

Choose a reason for hiding this comment

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

I personally agree with using p_ though the general GDScript style omits them

# Now create our uniform set so we can use these textures in our shader
texture_set[i] = _create_uniform_set(texture_rd[i])

func _render_process(p_next_texture, p_add_wave_point, p_texture_size, p_damp):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func _render_process(p_next_texture, p_add_wave_point, p_texture_size, p_damp):
func _render_process(next_texture, add_wave_point, texture_size, damp):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar as the discussion above, I've had to get a little creative with renaming these parameters because some shadow member variables. In this case the argument I made up above fully stands. next_texture,add_wave_point and damp can change in the main thread in preparation for the next frame being rendered, while the execution of _render_process in the render thread works with the data of the previous frame, hence we pass these as parameters.

compute/texture/water_plane/water_plane.gd Outdated Show resolved Hide resolved
@BastiaanOlij BastiaanOlij force-pushed the add_compute_texture branch 2 times, most recently from 0d46fe9 to 5d19534 Compare October 23, 2023 02:03
@BastiaanOlij
Copy link
Contributor Author

@AThousandShips I think I got all your changes :) I've left two items unresolved even though I've fixed things up because it is important to capture the feedback.

There might be more here we want to document or capture in some way as it is easy for users to ask the question, "why are these parameters, why aren't we using the member variables directly?". It's a logical question to come forward if we don't prefix parameters and thus increase the risk of variables shadowing members which can lead to confusing code. This lies at the heart of why I always prefix my parameters with p_, it makes it far more easy to read that we're not dealing with a member variable of the same name and that there must be reason to the madness of reusing a name.

Multithreading and what changes from frame to frame and how you can break things if you don't think this through are important especially in examples such as these. Simple mutex protection wouldn't help you in this scenario.

@AThousandShips
Copy link
Member

Will take a new look!

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Style looks good!

compute/texture/water_plane/water_plane.gd Show resolved Hide resolved
compute/texture/water_plane/water_plane.gd Show resolved Hide resolved
compute/texture/water_plane/water_plane.gd Show resolved Hide resolved
@Calinou Calinou merged commit 5eed925 into godotengine:master Nov 17, 2023
1 check passed
@BastiaanOlij BastiaanOlij deleted the add_compute_texture branch November 26, 2023 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There should be a compute shader demo with more features
9 participants