-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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 3D Asset Import Dialog material editing #86430
base: master
Are you sure you want to change the base?
Conversation
You have a leak related to themes and text, though I'm not really sure why. Edit: I think it's related to constructing LabelSettings. |
5bc40c8
to
36c5094
Compare
@MewPurPur Tends to optimize the svg icons after the merges. |
I'll do another big svg optimization rundown at some later point, I'm busy these days. As long as they passed through svgcleaner they should be good enough. |
(edit 2: I confirm this issue ↓ is still there in 4.2) when I override the materials of mesh instance nodes using "Surface Material Override" (under MeshInstance3D in the node inspector), the materials are reset / cleared for ALL mesh instances using that mesh, in ALL scenes, whenever I re-import the mesh (and it has changed somehow since the last import). This is VERY limiting. A mesh can be used in a variety of materials combinations, but in its current state, Godot is more suited towards meshes with a fixed set of assigned materials (because materials assigned to the mesh in the import window stay assigned on the mesh itself, and don't reset anytime the mesh is updated) The "Material override" property (under GeometryInstance3D in the node inspector) doesn't get reset though, on mesh re-import, but the problem is it doesn't allow assigning different materials to different parts of a mesh, whereas "Surface Material Override" can. |
Did the behaviour change? As far as I know the ArrayMesh materials can change due to import, but MeshInstance3D's overrides don't change. |
Oops, you're right, I just tried and it does work indeed. Sorry. Has this been fixed in 4.2? |
No, the issue is still there in 4.2. |
Will try to pick up this PR in the coming week, sorry for the delay! |
36c5094
to
a127083
Compare
Implements [7238](godotengine/godot-proposals#7238) (for the most part). * Ability to select nodes, meshes or materials by clicking. * Moved mesh/material previews to the inspector area. * Ability to override materials and edit them directly in the inspector. * By default, shows overridden materials in the imported scene, with a button to disable them and show original materials. * Keeps compatibility with the old override format.
a127083
to
4a461df
Compare
Alright, rebased, applied suggestions and updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, it works as expected.
I noticed a few usability issues though:
- This tooltip says that it works with the right click, but it actually works by left-clicking from my testing. This makes me wonder if having two separate modes is even needed, since right-clicking doesn't seem to do anything in either mode.
- There's also a typo in "materials" in the tooltip.
- We have a lot of space available in the top bar, so maybe this icon-only button should become a CheckButton with the text Show Material Overrides:
I tested and apart from what Calinou already mentioned in general the additions for the most part work fine for what they are supposed to do. Since I regularly use high-detail models I encountered a few usability problems, primarily with the selection and info:
Now for both issues imo a color highlight of the surface, e.g. a stripe overlay, would work better imo than this "selection aabb". It can be both oc. Another issue is that with the information split between tree left, inspector right you now need to constantly switch your eyes between them for basic things.
The raycast for picking also has detail problems.
Might need a way to click&hold with a popup that gives you all the objects in a small radius to choose like the main editor viewport has. If this is too difficult to add consider rotating through very close surfaces on every new click instead of only every picking the first surface that passes the ray test. |
@smix8 AFAIK these things are out of scope for this PR. Outline selections (like in Blender or Unity) will most likely happen once we have a proper compositor, as right now this is not possible. There are other contributors willing to give more freedom to the camera to improve also navigation of the imported scene. But then again, this PR should be taken as a starting point for those kind of improvements, not as a solution for everything. |
If there's planned improvements that we know we want, I think it's good practice to open new issues for them so that they don't get forgotten. I've done it multiple times with particle work and it's worked very well for me. It also open the door to other people picking up the work and lifts pressure from a single contributor to take care of it all. RE: big selection trees: I think it'd be a good starting point to scroll the tree to the currently selected item? RE:object picking: I think once stencil is in we can use an ID buffer that will be a lot more efficient. Before that is in we can also have just a secondary viewport with a special ID material, but I think this is a later improvement. Should probably be tracked though. |
I'll try to get some videos reviewing the asset import dialog soon. Edited: https://nightly.link/godotengine/godot/actions/artifacts/1197764151 Recording.2024-02-06.125738.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the feature and the improved 3d asset importer seems to work. Did not dig into the details of the code as much.
Will try to get some comments from the skeleton bone preview tool pull request developer. Edited: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We reviewed the pull request today in discord. Review comments incoming.
As a positive note, this workflow looks really helpful especially for FBX files since users often need to assign textures on FBX materials during import and that currently requires extracting the FBX to a file. I do have some concerns so I am unsure if this is something we should get in for 4.3: Finally, we currently have a major issue with the import system related to hard references in the .import file which break on path renames. Those reference will cause godot to crash (PR #90097 fixes the crash by failing the import instead) but the underlying issue is we cannot parse the .import file at all, which will prevent the user from opening the adv importer and correcting the issue. Users are already hitting this with BoneMap since that is currently one of the few places that use direct object references from .import |
This is a huge improvement. Being able to replace a material with a custom one that has it's own shader for example feels like an important feature to have. This doesn't really seam to be possible today by using the extract materials functionality. Is there any estimation on when this can be merged? |
I think this is a huge step toward making the 3D import pipeline more useable and encouraging good workflows. I also think it can definitely be taken further. I agree with @lyuma:
A very common reason for overriding materials is to do exactly this; adjust the albedo, increase the emission, etc. I think this might be even more common than needing to change everything about the material. For me the main goal of this system should be to:
In other engines such as Unity, making adjustments is very barebones: you only adjust very general import settings with the ability to filter out animations. I think Godot has a huge opportunity with this Import Dialog to go much further and make it a solid part of the asset authoring pipeline. It already leans towards this by moving away from the inspector into a dedicated scene viewer. I hope in the future we can leave behind the "import settings" approach of Unity and into the "asset setup and configuration" approach where you can use this window to:
While I recognize that there are workarounds for everything listed above, they often require a good amount of extra time, and time added to an asset pipeline scales linearly with complexity. Also, it often requires full control over the pipeline, which is not a good thing in my opinion. Godot should - within reason - be able to accommodate pipelines that aren't build from start to finish optimally for Godot and for different ways of working, be it rapid prototyping, using 3rd party assets or just mix and matching. Plus these changes also make everything more transparent in terms of what is happening and will, in my opinion, encourage better workflows than what I currently see used by less experienced developers/artists. TLDR; I hope - with time - we can make the Asset Import Dialog the one-stop-shop for setting up everything about the asset, not leaving anything to the inspector afterwards and that it can reasonably support common use-cases such as remapping textures, tweaking material parameters, channel-packing and converting wrongful texture channels from other render setups into proper Godot PBR maps. |
Is there anything any of us can do to help get this landed? |
@PadraigK We urgently need to allow referencing ExtResource in .import files UID, and to fix the engine crash that happens if a .import file has a broken UID reference. if anyone feels inspired to help with either of the above issues (in particular, changing the ConfigFile / .import serialization to reference ExtResource by uid://), it would be much appreciated! Once that is fixed, I think everyone is in agreement to merge this PR. (If we merge this PR without the .import reference fixes, it means projects are going to be broken or crashy the moment one material goes missing.) |
@lyuma Thank you for pointing out the issues that are blocking this PR! I am interested in investigating this further and maybe contributing a patch. I have started searching for more information in the issues and proposals to try to find more information and I've joined the development chat. Please let me know |
Implements 7238 (for the most part).
Note: Adding preview sun/env will be left for another PR.
How it looks: