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

Added material_overlay property to MeshInstance (gles2 & gles3) #50823

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

fbcosentino
Copy link
Contributor

@fbcosentino fbcosentino commented Jul 24, 2021

Current problem

Applying overlay materials (such as momentary visual effects) on multi-surface meshes currently requires adding a next pass material to all the surfaces, which might be cumbersome when the material is to be applied to a range of different geometries. E. g. an enemy plays a VFX when receiving a blaster hit, but each enemy type is a different mesh, with different number of surfaces.

This also makes it not very trivial to use AnimationPlayer to control the material. In the mentioned example, controlling the effect via animations require either adding a different AnimationPlayer to each different type of mesh, with manually set animations for each of the surfaces (which means only prepared targets could show the effect), or - if a same AnimationPlayer-based scene is instanced in any of the affected enemies - a script iterating over the surfaces of the mesh assigning the material, and exposing to the AnimationPlayer somehow (e.g. via export var).

The material_override property is not an option as it works replacing the active material for the surfaces, not adding a new pass.

Solution

This commit adds the material_overlay property to GeometryInstance (and therefore MeshInstance), which is a Material affecting all surfaces like material_override does, but rendering as a new pass on top of the active materials, instead of replacing them.

image

Implemented in rasterizer of both GLES2 and GLES3.

Example

Test model: mesh with 3 surfaces, each with a different material

image

Test effect added as next pass in first surface - provides a single point of access for an AnimationPlayer but needs scripting to be assigned to other surfaces - or repetition in the animation itself.

image

Test effect added as material_override - direct interface with single material, but underlying materials are lost

image

Test effect using material_overlay - effect covers entire model preserving base materials and providing single access for animations

image

GIF below demonstrates using "Next pass" to stack 3 material effects in material_overlay, all controlled via a single animation in AnimationPlayer. This AnimationPlayer can be saved as branch scene and instanced in any affected MeshInstance during runtime as needed. The only scripting required is assigning the material to material_overlay, which is completely independent from mesh materials or number of surfaces.

material_overlay_1

Edit:

master counterpart PR now at #53069

@Calinou
Copy link
Member

Calinou commented Jul 24, 2021

This sounds like a good idea to me. To help get this merged faster, I would recommend opening a proposal that follows the template so we can discuss the feature itself separately from its technical implementation.

Also, we will have to figure out how to implement this in master once we agree on the implementation details.

@sairam4123
Copy link

Just tested your PR, everything works good.

Documentation's second paragraph seems to be not aligned properly.
image

Other than that, the PR is working good.

doc/classes/GeometryInstance.xml Outdated Show resolved Hide resolved
doc/classes/VisualServer.xml Outdated Show resolved Hide resolved
@fbcosentino fbcosentino requested review from Calinou and removed request for a team July 26, 2021 15:09
doc/classes/VisualServer.xml Outdated Show resolved Hide resolved
@fbcosentino fbcosentino requested a review from Calinou July 26, 2021 19:03
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.

Documentation looks good to me.

@fbcosentino
Copy link
Contributor Author

A check failed due to a blank line and spaces in a comment. Do I need to make another commit or do I just leave it as is?

@Calinou
Copy link
Member

Calinou commented Jul 26, 2021

A check failed due to a blank line and spaces in a comment. Do I need to make another commit or do I just leave it as is?

You need to remove trailing whitespace and follow clang-format guidelines. You can do so by amending your latest commit (git commit --amend and force-pushing (git push -f). Also, commits should be squashed into a single commit; see PR workflow for instructions.

@fbcosentino fbcosentino force-pushed the 3d-material-overlay branch from 96f968d to b7d17f3 Compare July 26, 2021 22:21
@fbcosentino
Copy link
Contributor Author

Done. Please forgive my clumsiness - first time PR'ing here

@fbcosentino fbcosentino requested a review from Calinou July 27, 2021 09:57
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me! Reduz and I discussed the proposal over on the godot chat and agreed that the feature is acceptable in theory.

The only outstanding issue before merging is adding this feature to 4.0. We want to avoid adding new features into 3.x that won't be added in 4.0. You mentioned earlier that you are unsure how this works in 4.0. I think it actually works pretty similarly and should not be much more work for you. But if you are having trouble, please reach out to me here or on the godot chat

@akien-mga
Copy link
Member

@fbcosentino Would you be available to port this feature to 4.0 in the master branch as requested? Or should another contributor do it?

@fbcosentino
Copy link
Contributor Author

I will try my best to avoid taking time from other contributors who I know are already busy, but if somebody else would be willing to do it I wouldn't mind at all.

@fbcosentino
Copy link
Contributor Author

Finally implemented in master - #53069

I'm terribly sorry for the delay on this. Darkness took me, and I strayed out of thought and time.

Copy link

@sairam4123 sairam4123 left a comment

Choose a reason for hiding this comment

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

All works, tested on my game now.

sairam4123 added a commit to SaiponathGames/Godot-Road-Builder-3D that referenced this pull request Oct 28, 2021
…e compile Godot 3.x with that pr to make this work.
@fbcosentino
Copy link
Contributor Author

Just saw the 3.4 rc 3 announcement, where it is said "With this third Release Candidate, we have frozen feature development and are nearly ready to release the stable version."

So this means this feature is not going?

@Calinou
Copy link
Member

Calinou commented Nov 3, 2021

Just saw the 3.4 rc 3 announcement, where it is said "With this third Release Candidate, we have frozen feature development and are nearly ready to release the stable version."

So this means this feature is not going?

Indeed, this will have to wait for 3.5 to be merged. It can't be merged in 3.4.1 either given patch releases are now reserved to bug fixes. Sorry for the inconvenience.

@akien-mga akien-mga modified the milestones: 3.4, 3.5 Nov 8, 2021
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Can you rebase this on latest 3.x and just confirm that it is still working with all the changes made since?

Once you confirm that it is good to go, we will move forward with merging it. We want to merge feature changes early in the 3.5 release cycle.

@akien-mga
Copy link
Member

Can you rebase this on latest 3.x and just confirm that it is still working with all the changes made since?

Bump :)

@fbcosentino
Copy link
Contributor Author

Rebased.

Applying overlay materials into multi-surface meshes currently
requires adding a next pass material to all the surfaces, which
might be cumbersome when the material is to be applied to a range
of different geometries. This also makes it not trivial to use
AnimationPlayer to control the material in case of visual effects.
The material_override property is not an option as it works
replacing the active material for the surfaces, not adding a new pass.

This commit adds the material_overlay property to GeometryInstance
(and therefore MeshInstance), having the same reach as
material_override (that is, all surfaces) but adding a new material
pass on top of the active materials, instead of replacing them.

Implemented in rasterizer of both GLES2 and GLES3.
@akien-mga akien-mga merged commit 4103b0b into godotengine:3.x Nov 16, 2021
@akien-mga
Copy link
Member

akien-mga commented Nov 16, 2021

Thanks! And congrats for your first merged Godot contribution 🎉

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.

5 participants