playcanvas / editor

Issue tracker for the PlayCanvas Editor
https://playcanvas.com/
154 stars 28 forks source link

[RFC] Conditional Script Attributes #1196

Open marklundin opened 5 days ago

marklundin commented 5 days ago

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.

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

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 commented 5 days ago

How would you know when to evaluate the expression?

marklundin commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

I'm not sure what the win would be

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

Maksims commented 5 days ago

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 commented 5 days ago

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.