- 
                Notifications
    
You must be signed in to change notification settings  - Fork 3.6k
 
FrameGraph: multi-valued task properties #17247
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
Conversation
| 
           Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).  | 
    
| 
           Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17247/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/17247/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/17247/merge#BCU1XR#0  | 
    
| 
           Devhost visualization test reporter:  | 
    
| 
           Visualization tests for WebGPU  | 
    
| 
           WebGL2 visualization test reporter:  | 
    
    
      
        1 similar comment
      
    
  
    | 
           WebGL2 visualization test reporter:  | 
    
| 
           @Popov72 Any update on what to do here?  | 
    
| 
           (Ideally we do not want more decorators as they are a big problem for side effect)  | 
    
          
 Well, in that case let's just close it. Maybe it can be revisited later (but without decorators, the implementation may be a bit tedious).  | 
    
This is a PoC, intended to gather feedback on this feature (is it useful? How can it best be implemented? etc.).
Feature
Currently, properties in tasks are simple properties, such as
public numCascades: number;in theFrameGraphCascadedShadowGeneratorTaskclass.The idea is to be able to set a value for a property based on a condition. Of course, we could do this in user code:
This is a bit tedious, and the code can quickly become cluttered with numerous conditions if the frame graph has a high degree of variability (some parts of the graph can depend on the engine type, others on globalQuality, shadowQuality, ... parameters, etc.).
In this PoC, I suggest adding an additional variable
numCascadesMulti, which allows you to define the property as follows:When the graph is built, all multi properties of all tasks are parsed and evaluated: the first entry that returns true will set its value as the property value.
In this way, the variations are closely linked to the property and are clearly part of the graph itself, rather than additional code.
In addition, a new property
public available?: () => boolean;has been added to the task class: if the property is defined and the function returns false, everything behaves as if the task were not in the task list: it will neither be built nor executed.Example of use
Let's imagine that we want to display volumetric lighting only if the globalQuality parameter is set to “high”:
Without the PR:
With the PR:
When you change the value of globalQuality, in the first case, you must reset the frame graph and re-execute the function that creates it. In the second case, you just need to execute
frameGraph.build(). Furthermore, I think the code is more readable in the second case, and even more so if additional conditions are added to the code.Implementation
You must use a decorator to indicate that a property must have a corresponding “multi” property:
The decorator creates the
numCascadesMultiproperty. However, in order for the property to be available in IntelliSense, I had to declare it in the class:I don't know if there's a way to make it available via IntelliSense without having to add this declaration...
The generator can take an additional parameter, which is the name of the “setter” function, a function that will be called when the property changes (in case you need to execute additional code when this happens):
Note that there are no breaking changes,
numCascadesstill exists, and if you assign a value to it, this value takes precedence over what you might have assigned tonumCascadesMulti: the setter fornumCascadesdeletes theFrameGraphTaskPropertyinstance created whennumCascadesMultiwas first accessed.