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

Add visibility conditions for SSAO, Bloom, Grading, Vignette, Fringin… #7347

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

marklundin
Copy link
Member

Added conditional attribute tags to the camera-frame.mjs to show and hide them

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@marklundin marklundin self-assigned this Feb 12, 2025
@@ -108,45 +106,54 @@ class Ssao {
*/
type = SsaoType.NONE;

/**
* @visibleif {type !== 'none'}
Copy link
Contributor

@willeastcott willeastcott Feb 12, 2025

Choose a reason for hiding this comment

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

@mvaligursky This is another reason why I wanted an enabled property on SSAO. It's gonna look quite incongruous against the other effects in the Editor UI (where everything else can be toggled in the effect header bar).

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed on this. We could change the script to expose it as two booleans and combine those to pass to the engine.

Copy link
Member Author

Choose a reason for hiding this comment

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

could we have a type=combine|lighting and a separate enabled field?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, feel free to change this script

@@ -158,6 +165,7 @@ class Bloom {
enabled = false;

/**
* @visibleif {enabled}
Copy link
Contributor

@willeastcott willeastcott Feb 12, 2025

Choose a reason for hiding this comment

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

This is really rather elegant! 👍

@marklundin marklundin merged commit ad77cef into main Feb 12, 2025
7 checks passed
@marklundin marklundin deleted the feat-camera-conditional-attrs branch February 12, 2025 15:45
@Maksims
Copy link
Collaborator

Maksims commented Feb 13, 2025

Please refer to this issue: playcanvas/editor#1264
We don't hide stuff in Editor almost ever. The only exceptions is where selected field has related fields that only relate to it, for example based on fog type, you have different settings.
Also, we only disable things where changing them is simply not possible, for example in Settings > Layers, you are not able to change default layers name. But if changing field is conditionally possible, for example if render component is enabled its field is still relevant - then when render component is disabled we do not disable it UI, as editing it would require enabling/disabling component, which is unnecessary extra movement for the user.

So please, refer to the issue described above, and lest get UX consistent within Editor.

@marklundin
Copy link
Member Author

We show/hide fields in nearly all component inspectors (Light, Sound, Particle System, Camera, Render, UI). In addition we also enable/disable fields in Light Component.

Linking for reference playcanvas/editor#1264 (comment)

@Maksims
Copy link
Collaborator

Maksims commented Feb 13, 2025

We show/hide fields in nearly all component inspectors (Light, Sound, Particle System, Camera, Render, UI). In addition we also enable/disable fields in Light Component.

If the choice of related fields depend on the type of a related field, then yes. In some edge cases like "shadow" also, but in majority cases, especially when disabling whole component - we do not hide fields, as you have it there for a reason. If you need to remove it - you have removal for that. So hiding fields based on enable/disable of a block - is not used practice. Otherwise we would hide position, rotation, scale, etc fields of Entity if it was disabled?

Same applies to all components - their enabled/disabled state, does not hide their content, as you want to adjust it regardless of its current enable/disable state. It is only for things that you are very unlikely to dynamically change, for example changing fog type - is very unlikely.

So for post-effects - this is something you want to adjust, regardless if it is enabled or disabled in Editor. Because user might enable it in settings later. That is why hiding it - is not the way. Minimizing it when selected if it is disabled - is way better approach, like we do with material inspector - we minimize blocks when they don't have related data (same as not enabled), but we still allow you to go in and see fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants