openfx icon indicating copy to clipboard operation
openfx copied to clipboard

Require hosts to call Instance Changed action after loading a project

Open garyo opened this issue 10 months ago • 29 comments

Open Effects Proposal for Standard Change

Please read the contribution guidelines first.

Standard Change Workflow

  • [x] Create proposal as issue (you're doing this now!)
  • [x] Tag this issue with standard change tag
  • [ ] Identify subcommittee: at least one plug-in vendor, and at least one host
  • [x] Discuss the idea in this issue
  • [ ] Write new or updated code and doc
  • [ ] Publish updates as a pull request (ideally on a feature/PROPOSAL-NAME branch)
    • [ ] Make sure that PR references this issue number to keep them in sync
    • [ ] Discuss and review code in the PR
    • [ ] Meet all requirements below for accepting PR
  • [ ] When subcommittee signs off and other members don't have any further review comments, maintainer merges PR to master which closes PR and issue

Requirements for accepting a standard change:

  • [ ] Header files updated
  • [ ] Documentation updated
  • [ ] Release notes added
  • [ ] Compatibility review completed
  • [ ] Working code demonstrated with at least one host and one plugin
  • [ ] At least two members sign off
  • [ ] No further changes requested from membership

Summary

Currently a host may or may not call instanceChanged after loading a saved project. This means plugins do not get an opportunity to update the enabled status of params or take other actions based on the input clips and/or param values. This proposal would require conforming hosts to call kOfxActionInstanceChanged after loading a project containing OpenFX plugin instances. This call would be with type=kOfxTypeClip. kOfxTypeParameter would also make sense but I suggest we just use Clip, and plugins can do whatever they need to.

As for kOfxPropChangeReason, should we recomend kOfxChangeUserEdited since loading a project is a user-initiated action? We could add a new reason, kOfxChangeProjectLoaded if that is needed, to distinguish these events from others, but since some hosts are already doing this, I don't think we need to make them change their behavior.

Motivation

Without this, plugins can't set their param enabledness or some overlay features because nothing is called after createInstance. A plugin has no way to know it's been updated (clip connected, params modified) after the instance was created.

Problem

This would enable plugins to show proper param status after a project is loaded.

Impact

This should have no impact on existing plugins, since some hosts already call instanceChanged after loading a project, and a plugin should respond to instanceChanged whenever it happens.

It does impact hosts, because any conforming host will need to make this call, if it doesn't already.

Documentation Impact

This is primarily (or entirely) a documentation/standard change. No change to the existing API.

Stakeholders

Both plugins and hosts will benefit from this by improving UI behavior.

Discussion

garyo avatar Mar 29 '25 14:03 garyo

  • removing, linking to wrong one :)

revisionfx avatar Mar 29 '25 23:03 revisionfx

You could merge #180 with this

I'm not sure how that's related -- can you say more, @revisionfx ?

garyo avatar Mar 31 '25 14:03 garyo

Thinking more about this issue, I notice that instanceChanged can't get source images either. Would it make sense to create some way to get a clip's image dimensions at the current time in instanceChanged? Thoughts, anyone?

garyo avatar Mar 31 '25 14:03 garyo

What I was saying - we can't place initially position 2D param based on image dimension without adding a button ask user to press so we can get a UI callback to set that.

revisionfx avatar Mar 31 '25 16:03 revisionfx

Thinking more about this issue, I notice that instanceChanged can't get source images either.

It's highly impractical for clipGetImage() to be allowed - that's generally an expensive operation, unsuitable for this action.

Would it make sense to create some way to get a clip's image dimensions at the current time in instanceChanged? Thoughts, anyone?

Do you mean permitting clipGetRegionOfDefinition()? That's probably viable.

barretpj avatar Apr 01 '25 10:04 barretpj

Thinking more about this issue, I notice that instanceChanged can't get source images either. ... Do you mean permitting clipGetRegionOfDefinition()? That's probably viable.

Ah, great idea, @barretpj . Yes, that would work for me (and maybe it's already working to call that from within instanceChanged; I don't know). I know OpenFX allows clip images to vary in size over time, and this would support that (since it takes the current time) without having to actually compute the image pixels.

@revisionfx would that work for you?

garyo avatar Apr 01 '25 14:04 garyo

yes, parsing notes

"in Baselight the Source clip is disconnected during actionCreateInstance()" "crash in Nuke before 12.6"

revisionfx avatar Apr 01 '25 14:04 revisionfx

I'm not saying clipGetRegionOfDefinition() would work during instanceChanged in Baselight. Certainly during createInstance() it would be very unlikely to work.

barretpj avatar Apr 01 '25 15:04 barretpj

In Assimilate Scratch we call InstanceChanged when an input clip is connected. When CreateInstance is called the clips are not connected.

Guido-assim avatar Apr 01 '25 16:04 Guido-assim

Per discussion in the TSC meeting: this proposal will require hosts to call instanceChanged with reason=clip when a clip is connected, even if the clip connection is the result of loading a project. It will also mandate that clipGetRegionOfDefinition must work in instanceChanged, though perhaps it already does.

garyo-gpsw avatar Apr 01 '25 18:04 garyo-gpsw

Just for curiosity, if instanceChanged is called when loading a project right after creating the plug-in, and that instanceChanged in turns set other parameter values (e.g: a Choice parameter value that controls the value of other parameters. For example a Preset choice controlling a Double2D, but then user can set keyframes on that Double2D, the preset just serves as initialization). In this scenario, the plug-in probably doesn't want to update the other parameters because it may override the keyframes.

I was always assuming (maybe it's not written anywhere, I don't remember) that the createInstanceAction must be called after loading all parameter values, so that the plug-in can restore enabledness and other states there.

If you impose the host to call instanceChanged when loading a parameter, this implies 2 things:

  • createInstanceChanged will no longer be called after loading parameter values (I know of some plug-ins that expect things to be set already)
  • the plug-in has no way (that I can see) to distinguish between instanceChanged called as a result of user action vs project loading action.

Maybe add a ProjectLoadingReason so that the plug-in can ignore if necessary ?

MrKepzie avatar Apr 14 '25 08:04 MrKepzie

That's how I originally meant it to work. Calling 'instancedChanged' on each parameter when you load a plugin can cause all sorts of weirdnesses for more complex plugins as you mention. "createInstance" was meant to be called after all the parameter values were loaded and the plugin does what it needs to with the state of the plugin.

On Mon, 14 Apr 2025 at 09:47, Alexandre Gauthier @.***> wrote:

Just for curiosity, if instanceChanged is called when loading a project right after creating the plug-in, and that instanceChanged in turns set other parameter values (e.g: a Choice parameter value that controls the value of other parameters. For example a Preset choice controlling a Double2D, but then user can set keyframes on that Double2D, the preset just serves as initialization). In this scenario, the plug-in probably doesn't want to update the other parameters because it may override the keyframes.

I was always assuming (maybe it's not written anywhere, I don't remember) that the createInstanceAction must be called after loading all parameter values, so that the plug-in can restore enabledness and other states there.

If you impose the host to call instanceChanged when loading a parameter, this implies 2 things:

  • createInstanceChanged will no longer be called after loading parameter values (I know of some plug-ins that expect things to be set already)
  • the plug-in has no way (that I can see) to distinguish between instanceChanged called as a result of user action vs project loading action.

Maybe add a ProjectLoadingReason so that the plug-in can ignore if necessary ?

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/openfx/issues/184#issuecomment-2800916430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOIGQAQM27I77VFWNWFDIT2ZNY2XAVCNFSM6AAAAAB2BNYU42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMBQHEYTMNBTGA . You are receiving this because you are subscribed to this thread.Message ID: @.***> MrKepzie left a comment (AcademySoftwareFoundation/openfx#184) https://github.com/AcademySoftwareFoundation/openfx/issues/184#issuecomment-2800916430

Just for curiosity, if instanceChanged is called when loading a project right after creating the plug-in, and that instanceChanged in turns set other parameter values (e.g: a Choice parameter value that controls the value of other parameters. For example a Preset choice controlling a Double2D, but then user can set keyframes on that Double2D, the preset just serves as initialization). In this scenario, the plug-in probably doesn't want to update the other parameters because it may override the keyframes.

I was always assuming (maybe it's not written anywhere, I don't remember) that the createInstanceAction must be called after loading all parameter values, so that the plug-in can restore enabledness and other states there.

If you impose the host to call instanceChanged when loading a parameter, this implies 2 things:

  • createInstanceChanged will no longer be called after loading parameter values (I know of some plug-ins that expect things to be set already)
  • the plug-in has no way (that I can see) to distinguish between instanceChanged called as a result of user action vs project loading action.

Maybe add a ProjectLoadingReason so that the plug-in can ignore if necessary ?

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/openfx/issues/184#issuecomment-2800916430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOIGQAQM27I77VFWNWFDIT2ZNY2XAVCNFSM6AAAAAB2BNYU42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMBQHEYTMNBTGA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

bruno-j-nicoletti avatar Apr 14 '25 11:04 bruno-j-nicoletti

"createInstance" was meant to be called after all the parameter values were loaded and the plugin does what it needs to with the state of the plugin.

Yes, but this is about clips. At least in some hosts (see above, and also Resolve) the inputs are not connected at createInstance time -- at least that's my understanding. When a saved project is loaded, at least in those hosts, the input is silently connected but no instanceChanged action is called. (And I'm not sure If the plugin param enabled state depends on the input clips in any way (dimensions, color space, etc.) the plugin doesn't hear about its input clip til the user changes something else.

My use case would also be satisfied if the spec would guarantee that when loading a saved project, any input clips are connected before the host calls createInstance, and that clipGetRegionOfDefinition (and other clip properties) works on those clips in createInstance (as well as setting kOfxParamPropEnabled of course).

garyo avatar Apr 14 '25 11:04 garyo

Then I would say it's just a specification issue for the host implementation. These mentionned hosts should conform to it. Calling createInstanceAction wthout having clips connected is bound to bring trouble anyway as the plug-in is in an intermediate undefined state that cannot rely on any clip state.

Also a side discussion which belongs to another issue anyway, but I never really like the idea of Clip vs Parameter. I prefer having Clips being just Image Parameters, as it makes things much trivial to maintain and reason, especially for nodal graphs: the "input" of a clip/parameter can be e.g: a parameter link to another parameter in the host (many hosts have a concept of parameter linking) or e.g: an "expression" (or function) that generates a value for this parameter. I saw in another issue that people started so suggest having "Camera" suite behaving like clips... which to me feels wrong as it is just another type of parameter with a different type of data.

MrKepzie avatar Apr 14 '25 13:04 MrKepzie

Then I would say it's just a specification issue for the host implementation.

Yes, that's exactly what this issue is about. Refining the spec so it is clear what hosts must do.

However, I note that clipGetRegionOfDefinition requires a time argument, and the spec says that in createInstance there are no inArgs so there is no way to get kOfxPropTime in that action. So if we would like to go this way rather than instanceChanged, we would have to add inArgs to createInstance, containing the current time (I think it would be OK to fail if the instance is not yet associated with a clip or timeline, in which case I'd expect a later call to instanceChanged). But I'm guessing this is not going to be easy for some hosts.

Thoughts?

garyo avatar Apr 14 '25 13:04 garyo

Either add the kOfxPropTime to the inArgs of createInstanceAction (which is weird tbh) or comment that the timeline suite is expected to return the current time properly for a plugin within createInstance. I would document that all state should be loaded on host side before calling any createInstance action, which may look like a 2 pass function:

  1. Re-create all effects, clips and whatnots and connect them together. Also load any other timeline and relavant state in the project.
  2. Call createInstance action on all effects created in 1)

If a host cannot conform to this loading scheme, it should probably then call clipInstanceChanged on the clips, with a reason "LoadingProject"

MrKepzie avatar Apr 14 '25 13:04 MrKepzie

Using the timeline suite is a good idea, it seems to me.

So the proposed spec change(s) might be:

When a host creates a plugin instance from a saved project with a connected clip, the host must either:

  • connect all params and clips to an effect before calling createInstance, and support the OfxTimeLineSuite so the effect can call timelinesuite.getTime in createInstance (so the plugin can call clipGetRegionOfDefinition), OR:
  • call instanceChanged after createInstance and after the host has hooked up all clips, with property kOfxPropType = kOfxTypeClip and kOfxPropChangeReason = kOfxChangeProjectLoaded (this #define will be a new addition to the spec), and ensure that clipGetRegionOfDefinition works in that action.

I personally won't care about the reason, just that a clip was changed. Does this change look good to everyone? Host vendors especially please weigh in: @barretpj @Guido-assim etc.

garyo avatar Apr 14 '25 14:04 garyo

In Assimilate Scratch first all the params are connected, then createInstance is called, then the clips are connected followed by a instanceChanged call. timelinesuite.getTime would not return a meaningful result in createInstance, probably an error, because at that point you are not in a timeline playback situation yet. We call instanceChanged every time a (input) clip is connected or changed. I am not sure why you would need the kOfxChangeProjectLoaded, what would you do differently in the plugin from kOfxChangeUserEdited? What is the actual use case of calling clipGetRegionOfDefinition in createInstance or instanceChanged?

Guido-assim avatar Apr 15 '25 08:04 Guido-assim

For me, this is fine. In your host, it sounds like there's no difference between loading a project with a plugin and adding the plugin to a project and then hooking up its input. As I mentioned, I don't care about the kOfxChangeProjectLoaded reason -- I'll treat it just like the user changed it. As far as I'm concerned, they're the same.

My use case: I have certain params and param groups that should only be enabled when the input clip is of a certain aspect ratio (and other params have certain values). In your host, I can't know that at createInstance time because there's no input clip yet. But when you call instanceChanged I'll be able to get the source clip's RoD and other param values, and enable/disable the params properly. What I will do is first check for the input clip in createInstance; if it's there, I'll get the other param values and try to enable/disable params based on that info. If that doesn't work (no clip connected), I'll also do the same logic in instanceChanged.

If a host doesn't call instanceChanged, and doesn't have the clips already hooked up in createInstance, then I can't disable the param groups properly until the user changes something else and I get an (unrelated) instanceChanged, so to the user it looks weird that some param groups suddenly got disabled for no apparent reason.

I think @revisionfx 's use case is similar, but instead of enable/disable he wants to set a param value:

we can't place initially position 2D param based on image dimension without adding a button ask user to press ...

Pierre, does this proposal work for you?

garyo avatar Apr 15 '25 13:04 garyo

I still think this proposal is the way to go, but I will point out this sentence in the spec for kOfxActionCreateInstance in the "preconditions" section:

Pre: the instance is fully constructed, with all objects requested in the describe actions (eg, parameters and clips) have been constructed and have had their initial values set. This means that if the values are being loaded from an old setup, that load should have taken place before the create instance action is called.

So strictly, according to the spec, clips should be hooked up before calling createInstance. But as I said, the proposed change (as updated in https://github.com/AcademySoftwareFoundation/openfx/issues/184#issuecomment-2801817755) gives hosts more flexibility.

garyo-gpsw avatar Apr 21 '25 13:04 garyo-gpsw

@garyo-gpsw - the only special case I see is it's valid in a node graph to add an effect without it being connected to a clip yet. In such case the UI is populated with the effects controls. One would assume it gets an instance changed when clip is connected.

revisionfx avatar Apr 21 '25 14:04 revisionfx

I don't think we need a new Change reason; Silhouette uses kOfxChangePluginEdited and does it for each parameter and clip.

fxtech avatar May 06 '25 15:05 fxtech

I don't think we need a new Change reason; Silhouette uses kOfxChangePluginEdited and does it for each parameter and clip.

Is kOfxChangeUserEdited not more logical. The user/host attached the clips not the plugin itself. In Scratch we use kOfxChangeUserEdited. I see kOfxChangePluginEdited only being used in some overlay pen/mouse movement handling.

Guido-assim avatar May 06 '25 19:05 Guido-assim

It's a good point, Guido. Even though the host hooked up the clip(s), it's kind of like the user did it -- just in a previous session. Thoughts? Once this is decided I'll update the PR #191.

garyo avatar May 06 '25 19:05 garyo

The reason I went with kOfxChangePluginEdited is kOfxChangeUserEdited implies that the user directly fiddled with something. For example, I have a popup menu that acts like a preset selector - picking something from the list causes those "preset" values to be loaded into other parameters. If I send a UserEdited instance changed during project load, that code will trigger again and override any changes the user made after first twiddling it.

fxtech avatar May 07 '25 14:05 fxtech

Would it matter for kOfxTypeClip what the reason is? The parameters we have already set up before the CreateInstance call so we don't do an instance changed for that on project load.

Guido-assim avatar May 07 '25 14:05 Guido-assim

Would it matter for kOfxTypeClip what the reason is? The parameters we have already set up before the CreateInstance call so we don't do an instance changed for that on project load.

I don't think it would matter for clips.

fxtech avatar May 07 '25 15:05 fxtech

OK -- sounds like @fxtech doesn't care about the reason since the change event is on a Clip, and @Guido-assim is already using kOfxChangeUserEdited, and I also don't care about the reason, so let's go with kOfxChangeUserEdited since it's already out there in at least one host. I'll make that update in the PR.

garyo avatar May 08 '25 12:05 garyo

I switched Silhouette to pass kOfxChangeUserEdited for clip hookups.

fxtech avatar Jun 03 '25 15:06 fxtech