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

Improve the material preview in the inspector #99927

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ydeltastar
Copy link
Contributor

@ydeltastar ydeltastar commented Dec 2, 2024

The current preview setup is a clear color with no sky so PBR effects can't be visualized correctly and the material looks very different from what it would look in a scene.

I made the default preview use an Environment similar to the default 3D view's environment with a procedural sky. A floor can be toggled to preview reflections too using a reflection probe. The environment and floor material can be configured in rendering/environment/material_preview/...

Before After
image image
image image

@ydeltastar ydeltastar requested review from a team as code owners December 2, 2024 15:36
@@ -2458,6 +2458,12 @@
<member name="rendering/environment/defaults/default_environment" type="String" setter="" getter="" default="&quot;&quot;">
[Environment] that will be used as a fallback environment in case a scene does not specify its own environment. The default environment is loaded in at scene load time regardless of whether you have set an environment or not. If you do not rely on the fallback environment, you do not need to set this property.
</member>
<member name="rendering/environment/material_preview/environment" type="String" setter="" getter="" default="&quot;&quot;">
[Environment] that will be used in the inspector's material preview. If this is not set, a default environment will be used instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[Environment] that will be used in the inspector's material preview. If this is not set, a default environment will be used instead.
Path to the [Environment] resource that will be used in the Inspector's material preview. If this is not set, a default environment will be used instead.

What's "a default environment will be used instead" in this context? I am to assume it's NOT the editor setting with the same name. May be worth being more specific.

Copy link
Contributor Author

@ydeltastar ydeltastar Dec 2, 2024

Choose a reason for hiding this comment

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

It's the material preview's own default environment. Not related to the default fallback or setting for the environment of the 3D scenes.

doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
@ydeltastar

This comment was marked as resolved.

@ydeltastar ydeltastar force-pushed the improve-material-preview branch from 73f1f86 to 34f8b4b Compare December 2, 2024 16:26
@ydeltastar ydeltastar requested a review from a team as a code owner December 2, 2024 16:26
@AThousandShips AThousandShips added this to the 4.x milestone Dec 2, 2024
@lander-vr
Copy link
Contributor

I'm super excited to see this PR, thanks for this! I'm really happy with how you've implemented this, it's great to have full access to the environment.

Works great both in the inspector and shader editor.

A couple of suggestions:

  • If possible, when loading a file for the custom environment and floor material, it should filter the files so only resources that are suitable are shown, as opposed to all files.
  • I think it might make sense to remove that secondary light source (or at least have it disabled by default). The environment now does the job of providing fill light, so I don't think it's necessary to keep it.
    image
  • I believe the new icon should have a dark outline to stay consistent with the other icons.

Some other less important things that I think might make sense in the context of this PR (Only if you'd be interested, of course):

  • I noticed that both lights enable themselves whenever you switch between different materials. This was already the case before, but if it's an easy fix it might make sense to add that to this PR too.
  • Adding a dolly movement to the camera. Since scroll is already used by the inspector itself, probably on ctrl+middle click so it follows the same inputs as the main editor. Unless it's possible to take over that scroll input. In the shader editor preview, scrolling wouldn't be an issue.

@ydeltastar
Copy link
Contributor Author

  • If possible, when loading a file for the custom environment and floor material, it should filter the files so only resources that are suitable are shown, as opposed to all files.

That's not possible because string path properties only filter by file extensions and resources all have the same extensions (tres, res) or multiple variations.
I would like to use resource properties over file paths since we could use the quick open dialog like in the inspector. But they appear to be avoided in the settings, the default environment and GUI theme also use paths, so I'm following the current design. Maybe open a proposal to replace file paths for actual resource properties in the settings.

  • Adding a dolly movement to the camera.

There isn't much use for zoom in this simple preview with basic shapes. I'm not sure if it's worth interfering with the inspector's scrolling or adding a shortcut that isn't utilized elsewhere.

  • I noticed that both lights enable themselves whenever you switch between different materials. This was already the case before, but if it's an easy fix it might make sense to add that to this PR too.
  • I think it might make sense to remove that secondary light source (or at least have it disabled by default). The environment now does the job of providing fill light, so I don't think it's necessary to keep it.

Previously it reset rotations when changing shapes or materials too. My goal is to keep the preview state while switching so lights should do too. Fixed.

@ydeltastar ydeltastar force-pushed the improve-material-preview branch from 34f8b4b to 6407c1d Compare December 4, 2024 16:57
@tetrapod00
Copy link
Contributor

Previously it reset rotations when changing shapes or materials too.

When I added the quad mesh (#97030), I intentionally chose a few things:

  • Rotation should reset when selecting another mesh shape. For the quad, it is desirable for the quad to be perfectly aligned with the view when selected, since it is used for VFX and other cases where you want to see the 0-1 UVs. However, maybe now that there is an environment, it is now more important to maintain a certain camera rotation when switching shapes than it is to reset the quad to be view-aligned.
  • Rotation should be clamped to reasonable values for the quad mesh (less than 90 degrees), so the quad is always visible. I actually changed my mind about this while implementing it; I originally thought it should be totally unclamped for the quad, and said so in the proposal (Add a quad mesh to the material preview in the inspector godot-proposals#10744). So changing to no horizontal rotation clamping, as you've done here, is a reasonable choice. However, if you do leave the horizontal rotation unclamped, I think that the rotations definitely should reset when clicking on each shape, so you have a way to "make the quad show up again". Or maybe there should be another button to reset the rotation?
  • Rotation should be remembered when switching between different selected materials, to make comparing between two materials easier. I'm not sure why you were seeing that rotations got reset when selecting other materials. On 4.4dev5, the rotation doesn't reset for me when toggling between two selected materials in the inspector.

However, while I did put some thought into these choices, they aren't the only reasonable choices to make, and the tradeoffs might have changed with the addition of an environment. I just wanted to give some context.

@ydeltastar
Copy link
Contributor Author

ydeltastar commented Dec 4, 2024

  • Rotation should reset when selecting another mesh shape. For the quad, it is desirable for the quad to be perfectly aligned with the view when selected, since it is used for VFX and other cases where you want to see the 0-1 UVs.
  • Rotation should be clamped to reasonable values for the quad mesh (less than 90 degrees), so the quad is always visible.

I think these should be left for the user to decide. Automatic resets hinder workflow, I'm always putting the rotating back where it was. Hardcoded restrictions limit usage, for example, you may want to visualize disabled back-face culling which is a material setting but you can't see the back of the quad because rotating was clamped. (Note: cull modes don't update unless the preview is recreated, but it also happens in the 3D view, bug not related to the PR).

Rotation should be remembered when switching between different selected materials, to make comparing between two materials easier. I'm not sure why you were seeing that rotations got reset when selecting other materials.

That must be the case. Changing shapes did reset it and I did an overhaul on the logic so I probably remember it wrong about materials. Making comparing consistent is also one of the reasons I decided not reseting rotation.

@tetrapod00
Copy link
Contributor

tetrapod00 commented Dec 4, 2024

Yeah, I agree that cull_disabled materials are a good reason to allow unlimited rotation! So I'm not opposed to changing the behavior here.

In that case I would like to see some way to reset the rotation, but I'm not sure what the best UI is. I also don't think that it necessarily has to be implemented in this PR. IMO it's okay to simply remove rotation clamping and rotation resetting, and if there is enough demand to reset the rotation, add it back in the future. But others may disagree.

Off the top of my head, possible ways to reset rotation:

  • Add another button just for this purpose.
  • If a certain shape is selected, clicking on that shape icon will reset the rotation.
  • Double clicking on the shape icon will reset rotation.
  • Double clicking anywhere on the preview (but not on any button) will reset the rotation.
  • CTRL+clicking anywhere on the preview will reset the rotation.

@ydeltastar
Copy link
Contributor Author

  • If a certain shape is selected, clicking on that shape icon will reset the rotation.

I like this option since it avoids more buttons. Implemented it.

@ydeltastar ydeltastar force-pushed the improve-material-preview branch 2 times, most recently from 22f52b7 to 01df955 Compare December 4, 2024 23:51
@lander-vr
Copy link
Contributor

If a certain shape is selected, clicking on that shape icon will reset the rotation.
Double clicking anywhere on the preview (but not on any button) will reset the rotation.

These two make a lot of sense and seem intuitive to me.

@Calinou
Copy link
Member

Calinou commented Dec 5, 2024

Regarding the default sky colors, I wonder if we should go for a purely grayscale sky by default, like we do in the Advanced Import Settings dialog. That dialog uses a sky which is fully white at its top and fully black at its bottom, but we can use something that resembles the luminance of a real sky more closely too.

The reason for using a grayscale sky is to avoid skewing the reflections towards a certain color, which can bias certain materials as looking more blue than they really are. It can also make previews look misleading if your project never uses skies with a blue-ish hue. There are project settings available to use a custom preview environment either way, so if skewing the previews towards a certain color makes sense for your project, you'd still be able to do it.

sphere_switch->set_pressed_no_signal(true);
box_switch->set_pressed_no_signal(false);
quad_switch->set_pressed_no_signal(false);
cam_zoom = 2.973804;
Copy link
Member

Choose a reason for hiding this comment

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

Does this zoom value correspond to anything specific? There should be a comment above it, or it should use the value of a named constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from visible_height / sin(vfov) for precise camera framing.
I refactored the switch logic in the new push.

box_switch->set_pressed_no_signal(true);
sphere_switch->set_pressed_no_signal(false);
quad_switch->set_pressed_no_signal(false);
cam_zoom = 4.35f;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

quad_switch->set_pressed_no_signal(true);
sphere_switch->set_pressed_no_signal(false);
box_switch->set_pressed_no_signal(false);
cam_zoom = 2.973804;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@ydeltastar ydeltastar force-pushed the improve-material-preview branch from 01df955 to f1d1bd5 Compare December 5, 2024 18:04
@ydeltastar
Copy link
Contributor Author

The reason for using a grayscale sky is to avoid skewing the reflections towards a certain color, which can bias certain materials as looking more blue than they really are.

It is a good point for making it like neutral studio lighting. However, I want to note that it's a bad practice to use a non-realistic sky for PBR authoring since we should design expecting hue influences from the environment.

Any option is fine. I'm not particular about the defaults as long as they can be configured.

@fire
Copy link
Member

fire commented Dec 5, 2024

I want to boost the opinion that it is bad practice to use a non-realistic sky, too.

@ydeltastar ydeltastar force-pushed the improve-material-preview branch from f1d1bd5 to 5f74e42 Compare December 5, 2024 18:16
@lander-vr
Copy link
Contributor

I want to note that it's a bad practice to use a non-realistic sky for PBR authoring since we should design expecting hue influences from the environment.

Agreed, when working on materials I prefer something more representative of an actual lighting scenario rather than an unnatural gray, an outdoors setting is a good default. I think the blue-tint in the preview is definitely neutral and toned down enough, it misrepresenting hues shouldn't really be a concern.

Besides that, I think it's a more fun/nicer experience working on materials, and as a bonus makes the editor look a little nicer, if the preview lighting has a little bit of flair instead of being entirely gray.

@ydeltastar ydeltastar force-pushed the improve-material-preview branch from 5f74e42 to b734763 Compare December 5, 2024 22:00
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.

Improve material preview in the inspector
7 participants