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

[RFC] Conditional Script Attributes #1196

Open
marklundin opened this issue Sep 13, 2024 · 13 comments
Open

[RFC] Conditional Script Attributes #1196

marklundin opened this issue Sep 13, 2024 · 13 comments

Comments

@marklundin
Copy link
Member

marklundin commented Sep 13, 2024

Problem

It would be useful to have script attributes that only displayed under certain conditions. For example

class PostProcessing extends Script {
  /** @attribute */
  useBloom = true;
  
  /** 
    * @attribute
    * @range [0, 10]
    */
  bloomIntensity = 1;
}

Here we might only need to show the bloomIntensity slider to the user when useBloom is enabled.

There has been community demand for this:
#664
#278
https://forum.playcanvas.com/t/can-i-change-the-visibility-of-attributes-in-the-editor/36144/7
https://forum.playcanvas.com/t/can-we-change-the-script-attributes-by-the-type-of-an-enum/25865

Proposal

Implement a way to to declare dependencies between attributes

class PostProcessing extends Script {
  /** @attribute */
  useBloom = true;
  
  /** 
    * @attributeWhen {useBloom}
    * @range [0, 10]
    */
  bloomIntensity = 1;
}

The @attribtueWhen (TBD) accepts a conditional expression that is evaluated to determine it's visibility eg: @attributeWhen {usePost && useBloom}. The attribute control is enabled if the expression returns truthy.

Alternative tag names

  • @attribute {expression}
  • @attributeWhen {expression}
  •  /** 
       * @attribute
       * @dependsOn {expression}
       */
    
  •  /** 
       * @attribute
       * @visibleWhen {expression}
       */
    

Concerns

Any {expression} would need to be evaluated within the editor, and this is problematic if using eval() or new Function. eg @visbleWhen(while(true){}). Instead the expression should be evaluated in some minimal sandboxed environment

@slimbuck
Copy link
Member

How would you know when to evaluate the expression?

@marklundin
Copy link
Member Author

All expressions for a script would be evaluated in order when any one of it's attributes is changed in the editor. So if a user toggles a check box, just naively iterated over all other attributes with conditions, and evaluate them with the current data

@mvaligursky
Copy link

mvaligursky commented Sep 13, 2024

I like it. But consider you want to safely run eval() already, could this be made more generic? Something like this.

class PostProcessing extends Script {
  /** @attribute */
  useBloom = true;
  
  /** 
    * @attributeWhen {useBloom}
    * @range [0, 10]
    */
  bloomIntensity = 1;

  onInspector() {
     show(this.bloomIntensity, this.useBloom);
  }
}

@marklundin
Copy link
Member Author

The class is never actually initialized in the context of the editor. it would add a lot more complexity, and I'm not sure what the win would be

@mvaligursky
Copy link

I'm not sure what the win would be

other than "it would be amazing" I have nothing more specific at the moment :)

@Maksims
Copy link

Maksims commented Sep 13, 2024

I would definitely avoid using eval as much as possible. E.g.: post a link in forum/git, asking: "I have a bug, here is repro", users will follow it, and end up executing the XSS, as Editor is on playcanvas.com, not like Launcher.

The naming I would call "@visibleIf". it should be still an @attribute, but has a conditional visibility.
For the conditions, common use case is not just true/false (checkbox-dependant), but equal to value also: enums. E.g. show certain fields based on fog type.

For the conditions, it is possible to do some simple parsing, with some defined syntax that devs should follow. E.g. tags search with multiple arrays is implemented for assets already, we can do for conditions a similar thing.
Also, due to parsing, you will have a list of all property paths, that way you will be able to build an index, and toggle visibility only when a property within such index have changed. It is way more optimal especially for a larger scripts.

Also, need to consider JSON type attribute, with a schema, and how it will work there - relative paths.

@marklundin
Copy link
Member Author

marklundin commented Sep 13, 2024

I would definitely avoid using eval as much as possible.

Yep eval() is evil

For the conditions, common use case is not just true/false (checkbox-dependant), but equal to value

Yep idiomatic JS scoped to the current script @visibleIf {isOn || (intensity >= 1 && useBloom) } is probably the most intuitive without requiring custom dsl.

Also, due to parsing, you will have a list of all property paths, that way you will be able to build an index, and toggle visibility

Yes agreed

/** @interface */
class Enemy {
  hasWeapon = true
}

class Game extends Script {
  /**
   * @attribute 
   * @type [Enemy}
   */
   enemy

  /** 
   * @attribute
   * @visibleIf {enemy.hasWeapon}
   */
  displayWeapons
}

Also, need to consider JSON type attribute, with a schema, and how it will work there - relative paths.

Looking up parent paths (ie an object attribute being conditional to a parent attribute) might be strange eg @visibleIf {parent.path === true}. Also it becomes less self documenting and couples it to the parent. Maybe a solution to this can come later.

@LeXXik
Copy link

LeXXik commented Oct 9, 2024

Would be amazing to have. Our physics script attributes are a huge wall of options that is cumbersome to use.

@mvaligursky
Copy link

It'd be great to have access to all members of the script, even if individual parts of it are broken into interface classes. I'd want to access another interface class attribute from a different interface class.

Even better, maybe we could make all attributes of the entity available?

A simple syntax to access local attributes:
@visibleIf {enemy.hasWeapon}

Access other script attribute:
@visibleIf {entity.script.camera.clearColor}

@mvaligursky
Copy link

I would also like collapsible headers for interface classes, based on its enabled attribute. In the script I created, I have mutliple subsections that are available only when enabled is ticket. Ideally the enabled attribute would be on the header, and that would collapse when disabled.

Screenshot 2024-10-09 at 15 07 56

@marklundin
Copy link
Member Author

marklundin commented Oct 9, 2024

This could be an easy lift where we use similar UX to existing collapsible content. Simply show a toggle on the folder when the interface has an enabled property, and then close/disable the section.

image

@mvaligursky
Copy link

This could be an easy lift where we use similar UX to existing collapsible content. Simply show a toggle on the folder when the interface has an enabled property, and then close/disable the section.

image

Yep lets do it. Detect 'enabled' property for this.

@LeXXik
Copy link

LeXXik commented Oct 18, 2024

I would like to be able to show/hide attributes, based on the drop down menu selection. Similar to how default collision component behaves, when a collision type is selected. Might be similar or same to what @mvaligursky requested.

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

No branches or pull requests

5 participants