-
Notifications
You must be signed in to change notification settings - Fork 73
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
[F3D] Don´t hide properties that will get exported (specifically geo modes) #379
Conversation
I don't agree with this PR. When I redesigned the geometry mode UI (which was needed for F3DEX3, but applies to all microcodes), I found it confusing to remember which features depended on which other features, even for features I made myself. Hiding features which you cannot have without other features, e.g. things dependent on lighting, is an easy way to represent this; the alternative would be to have a ton of warnings and no structure in the UI. It is true that you can import and export a state which is not visible in the UI, e.g. texgen without lighting. However:
|
If you absolutely must have them visible--for example, because you want people to know where the texgen option is while they have lighting disabled, otherwise they can't find it--then I'd suggest you change the code so that instead of hiding the currently-invalid options, it grays them out (disables them) instead. Though that might be more confusing, as in our example texgen would be grayed out and checked, which has the semantic meaning of "this is required and you can't turn it off", rather than "this is stuck on but should not be". |
I think disabling works quite well, the identation makes whats happening pretty obvious I think, decided to not disable in world defaults and fixed my typo in the node update (g_texture_gen instead of g_tex_gen) |
Also world defaults ui is a broken in EX3 #370 |
if not getattr(settings, textOrProp): | ||
return None |
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.
Enable should not be a parameter, enable should be coming from this, that's the whole point of this function, to enable/disable the group based on whether the head of the group is enabled.
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.
Please fix one tiny issue, otherwise approved. I'm still not sure this is ideal but if it unblocks the world defaults stuff then it's okay.
fast64_internal/f3d/f3d_material.py
Outdated
c.prop(settings, "g_fresnel_color") | ||
else: | ||
inputGroup.column().label(text=f"Shade color = {shadeColorLabel}") | ||
inputGroup.label(text=f"Shade color = {shadeColorLabel}:") |
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.
Please remove the colon from the end of this line
There was two options in fixing this:
Not exporting hidden modes or always showing all modes
I went with the latter in this pr as its more logical, nonsense example but if a game always defaults to packed normals but doesn´t default to lighting, that should still be setable AND visible in the world settings. Or maybe one could use a hack that revolves around checking for an unused mode being on. In my opinion this is the best way to do it and it doesn´t add much bloat to the UI even in ex3.
I also fixed a bug where if texgen was on without lighting it would still emulate texgen, even though it was neither visible or correct