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

[3.x] Add ORMSpatialMaterial #76023

Merged
merged 1 commit into from
Feb 8, 2024
Merged

[3.x] Add ORMSpatialMaterial #76023

merged 1 commit into from
Feb 8, 2024

Conversation

Ansraer
Copy link
Contributor

@Ansraer Ansraer commented Apr 13, 2023

Adds an ORM material to 3.x, similar to what we have in master.
This allows users to combine occlusion, roughness, and metallic in a single texture, a common workflow used for games.

PS: please don't ask me how much time I wasted fighting with the classdb and the editor.


This PR was sponsored by Ramatak with 💚

@YuriSizov
Copy link
Contributor

Note that this technically breaks compat as it changes the inheritance chain and also moves some API to a different class. GDScript shouldn't care, and C#, from what I understand, doesn't care about it either, if the final API surface is the same. But it's worth to be cautious.

@lawnjelly
Copy link
Member

lawnjelly commented Apr 13, 2023

Yes, changing SpatialMaterial is quite compat breaking.

Not just core, but a lot of third party stuff relies on this. I was kind of assuming you would add ORMMaterial as an additional material type, rather than replace existing, or keep the SpatialMaterial name, whatever was the most appropriate.

EDIT: Ah ignore me, as Yuri says below, I didn't read through properly. Inheriting could work. 👍

@YuriSizov
Copy link
Contributor

@lawnjelly Currently it introduces the new class as a parent of SpatialMaterial, and moves some of the API to that new class. So technically, the API available to SpatialMaterial remains the same, it's just inherited in some cases. So it's not a complete breakage, but still something potentially incompatible with third party code at least.

@Ansraer
Copy link
Contributor Author

Ansraer commented Apr 13, 2023

Yeah, that's why it took me so long to write this PR. The problem is that I didn't want to have to rewrite (ok, copy-paste) pretty much the entire SpatialMaterial class with only two lines changed. Doing so would have resulted in a ton of needlessly duplicated code.
However, due to how the classdb currently works I couldn't really inherit from SpatialMaterial without breaking the editor either. This is my third attempt at this PR, my previous two attempts suffered from various visual glitches in the editor UI (especially the inspector).

I fully agree that this PR changes way too many files, but if I want to leave SpatialMaterial unchanged I would have to make ORMSpatialMaterial inherit it and then hack the editor anywhere Materials are used to support having a non-abstract parent class.
The current implementation works around that limitation and appears to work fine both when used with the editor or from a script.

Would be happy to go more in-depth and explain the different approaches I already tried if anyone is interested.

@Calinou
Copy link
Member

Calinou commented Apr 13, 2023

A different approach was attempted for 3.x in #26205, but it didn't bring efficiency advantages and was therefore reverted.

@Ansraer
Copy link
Contributor Author

Ansraer commented Apr 13, 2023

I don't think #26205 was doing quite the same thing I am doing here. Looking at the code it merely adjusted the defaults to make it easier to use orm textures with the default SpatialMaterial. My PR on the other hand changes the glsl code of the ORMMaterial.

@lawnjelly
Copy link
Member

lawnjelly commented Aug 6, 2023

Just starting to try this out. Would benefit from a test project, I'm hacking one together from the gltf sample damaged helmet.

I did notice when creating a new ORM material when I add the albedo it shows in the editor, but when I drag on an ORM texture it doesn't seem to update the editor. But when I tick "transmission" it starts to show, and then still shows when I untick. Which suggests something isn't getting updated after dragging on the ORM?

orm_helmet

I was also getting some incompatibilities in the Material perhaps when I saved from the PR version and then tried to load in vanilla Godot it didn't recognise Material3D but I guess that is expected. Still trying out. 👍

I'm hoping this would be super useful for 3.x for PBR games as it should be a lot more efficient than the current approach with separate textures.

Looking through the commit diffs it's kind of painful because most of it is the rename from SpatialMaterial to Material3D. Would be helpful if you can pinpoint the files to concentrate on. I expect this is why it has been tricky to review so far.

Maybe I can try locally renaming Material3D back to SpatialMaterial just to pinpoint the diffs of interest with the original code. 😄

@lawnjelly
Copy link
Member

Ok the trick with renaming Material3D back to SpatialMaterial worked as far as compressing the diffs. There aren't actually that many changes and it is mostly looking good to me.

Will need @clayjohn to review the shaders properly. I'll see if there's anything I can spot.

And I'll try and do more testing. Existing projects like 3rd person demo seemed to still work fine.

@Ansraer
Copy link
Contributor Author

Ansraer commented Aug 6, 2023

Yeah, I am sorry about that mass rename. I think I can vaguely remember that reduz wanted it to be renamed, but it has been a while so I might be wrong about that.

Thanks for letting me know about that editor bug, I will look into it. I can't really make any promises as to when I will get to it, I got some bad news the day after I returned from Valencia and my private life has been kind of a mess ever since. I took the next two weeks off to deal with things, but tracking that down might be a nice distraction if I find some spare time.

@lawnjelly
Copy link
Member

I got some bad news the day after I returned from Valencia and my private life has been kind of a mess ever since.

Don't worry real life stuff is more important! Make sure you are well. There will always be another day to polish up PRs.

@lawnjelly
Copy link
Member

Rendering meeting today:
We are all happy for this feature for 3.6 as it should improve things quite a bit, especially for mobile. 🙂

I'll try and test this a bit more, and if we decide on any changes @clayjohn has suggested I / other maintainers should push them if @Ansraer is unavailable so we can get this through. 👍

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 on a new project, it works as expected. Existing materials are also kept correctly when importing the 3D Material Testers demo (3.x branch).

Testing project: test_orm_material_3.x.zip
Material used in the testing project: https://www.cgbookcase.com/textures/round-metal-tiles-01/

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Wanted to get some movement on this with the deadline coming up. I had a look through a while ago and it looked ok, and the shader looks fine.

Could do with a check from @clayjohn too.

Needs a rebase for docs changes it looks like.

@lawnjelly lawnjelly modified the milestones: 3.x, 3.6 Feb 5, 2024
@lawnjelly
Copy link
Member

Bumping milestone to 3.6 as this looks good and should be possible to get in for 3.6 (if we can).

@akien-mga
Copy link
Member

Needs rebasing.

@akien-mga akien-mga merged commit 354404d into godotengine:3.x Feb 8, 2024
13 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

6 participants