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 2D instance shader parameters #97058

Closed
wants to merge 1 commit into from

Conversation

huwpascoe
Copy link
Contributor

@huwpascoe huwpascoe commented Sep 15, 2024

From KoBeWi to EIREXE to here. Issue #62943.

image
instvartest.zip

Now feature complete with the exception of compatibility renderer.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 15, 2024

Parameter list doesn't work properly. It's not updated right when modifying shaders and some operations may replace it with nulls:

godot.windows.editor.dev.x86_64_PfbFzWf1sD.mp4

Other than that they look functional, which is nice.

@KoBeWi KoBeWi added this to the 4.4 milestone Sep 15, 2024
@huwpascoe huwpascoe force-pushed the instance_uniforms_2d branch 2 times, most recently from cb7e597 to 49b0ac9 Compare September 16, 2024 16:33
@huwpascoe huwpascoe marked this pull request as ready for review September 16, 2024 16:33
@huwpascoe huwpascoe requested review from a team as code owners September 16, 2024 16:33
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 locally (rebased on top of master 99a7a9c), it works as expected in Forward+ and Mobile.

When running in Compatibility, you get a shader compilation error like in 3D (as expected):

SHADER ERROR: Uniform instances are not supported in gl_compatibility shaders.

image

Testing project: test_pr_97058.zip

PS: What is this checkbox for? It's present for both 3D and 3D per-instance uniforms. I can't uncheck it (nothing happens when I click it), regardless of whether the value is modified from its default or not.

image

doc/classes/CanvasItem.xml Outdated Show resolved Hide resolved
@huwpascoe
Copy link
Contributor Author

What is this checkbox for?

Don't know, I think it was like that in 4.2 as well.

@akien-mga akien-mga requested a review from clayjohn September 17, 2024 07:06
scene/main/canvas_item.h Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.h Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Sep 17, 2024

To update property list, you need to use notify_property_list_changed. Here's how it's done in 3D:

if (material_override.is_valid()) {
material_override->connect(CoreStringName(property_list_changed), callable_mp((Object *)this, &Object::notify_property_list_changed));
}

However modifying shader does not add the CanvasItem to _instance_update_list, I think it also has to be done manually.
The parameters no longer become null, so the list not updating is only an inconvenience now.

Also if you want the instance parameters to have revert button, you need to implement _property_can_revert() (return true if instance uniform exists) and _property_get_revert() (return default value of uniform).

@huwpascoe
Copy link
Contributor Author

Redid everything from the ground up, refactored all the common code into a class. Everything should work identically to the 3D instances now.

@huwpascoe huwpascoe force-pushed the instance_uniforms_2d branch 2 times, most recently from 736081e to db276ca Compare September 26, 2024 16:26
doc/classes/CanvasItem.xml Outdated Show resolved Hide resolved
@Chaosus
Copy link
Member

Chaosus commented Sep 26, 2024

Please squash the commits as it's required by our pipeline (see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html).

@huwpascoe
Copy link
Contributor Author

Please squash the commits

Do I need to do anything special to keep the other contributor's names or do I just squash?

@clayjohn
Copy link
Member

Please squash the commits

Do I need to do anything special to keep the other contributor's names or do I just squash?

You will need to have the "Co-Authored by:" lines for both of them (one is already there).

I suggest waiting to squash until you are finished with the PR and it is ready to merge.

servers/rendering/instance_uniforms.cpp Outdated Show resolved Hide resolved
servers/rendering/instance_uniforms.cpp Outdated Show resolved Hide resolved
servers/rendering/instance_uniforms.cpp Outdated Show resolved Hide resolved
servers/rendering/instance_uniforms.cpp Outdated Show resolved Hide resolved
servers/rendering/instance_uniforms.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.h Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.h Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_scene_cull.h Outdated Show resolved Hide resolved
@bunnybreaker
Copy link

Any idea on if/when this will get added to the main branch?

@huwpascoe
Copy link
Contributor Author

Any idea on if/when this will get added to the main branch?

Depends on #97340. Soon as it's merged, I'll submit the final revision and then it's in the hands of the maintainers.

Co-authored-by: Álex Román Núñez <eirexe123@gmail.com> and KoBeWi
@huwpascoe huwpascoe force-pushed the instance_uniforms_2d branch from c59b1bb to 6b34ceb Compare November 7, 2024 19:03
@huwpascoe
Copy link
Contributor Author

It's finished. But they want compat renderer support added.

Someone else can do that, I'm tired.

@huwpascoe huwpascoe closed this Nov 12, 2024
@AThousandShips AThousandShips removed this from the 4.4 milestone Nov 12, 2024
@Delsin-Yu
Copy link
Contributor

It's finished. But they want compat renderer support added.

Someone else can do that, I'm tired.

Is there any chance can we merge this and leave the compact renderer support for future contributors?

@KoBeWi
Copy link
Member

KoBeWi commented Nov 12, 2024

It will at least need a warning that Compatibility is not supported.

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.

10 participants