cocopon / tweakpane

:control_knobs: Compact GUI for fine-tuning parameters and monitoring value changes
https://tweakpane.github.io/docs/
MIT License
3.54k stars 87 forks source link

refresh() should (?) force invoke change event #362

Closed jkozniewski closed 2 years ago

jkozniewski commented 2 years ago

I've come to a scenario when I'm using global change event pane.on('change', (e) => {}) since I need to process the input in a bit more advanced way and can't simply bind to the properties of existing objects, since I'm operating on dynamic array of objects, so I keep the shared values in separate object that I can bind the inputs to and then using for loops in the event handler to iterate over all the objects in corresponding arrays to actually apply the values.

The problem is - if I create a new array of objects and I want to re-apply all the values there seems to be no way to do so since the refresh() method invokes 'change' event ONLY when there is a change in value - I mean, I understand it's called 'change' for a reason, but for a scenario when I just want to re-apply the values to new set of objects by processing them in global 'change' handler there seem to be no straightforward way to do so... Or am I missing something ? If not - I guess such feature called for example forceRefresh() or refresh( forceUpdate )(?) would be much welcome for more advanced scenarios like this.

jkozniewski commented 2 years ago

ok.. so the temporary hack for now looks like this:


const settingsChangeHandler = (e) => {
  // do something based on passed presetKeys and values in fake TpChangeEvent
  // like you do normally due to user interaction...
}

pane.on('change', settingsChangeHandler )

const preset = pane.exportPreset() 

for (const [key, value] of Object.entries(preset)) {
   settingsChangeHandler( { presetKey: key, value: value } )
}

that effectively re-applies all the current settings controlled via the panel

cocopon commented 2 years ago

Sorry, I still cannot understand the situation. Could you share a more concrete example?

jkozniewski commented 2 years ago

ok, I'll try to make some minimal example, but maybe for now let me ask how would you approach a scenario when you can't bind the parameter directly to a particular object, but rather you have some arbitrary changing collection (array) of the objects sharing same property and you want to always keep all of those object in sync with what's currently set in the panel, regardless if some are added dynamically or removed during the application livecycle ?

cocopon commented 2 years ago

I feel that your case is complicated and I cannot understand the situation from the text (maybe it is because I'm not good at English). So I'd like to see the situation with actual code.

RedSparr0w commented 2 years ago

Say for example I was changing some css variables when ever some of my values changed (which I allow users to pre-input via url params)

These wouldn't be updated until the value get's changed within the panel.

Example codepen (assume the default settings are the values supplied by the user)

Example scripts: current (no ability to trigger on change event with `refresh()`): ```js // Our default settings const Settings = { // Setup username: 'RedSparr0w', // Display background_color: 'rgb(0, 0, 255)', message_color: 'rgb(255, 0, 0)', username_color: 'rgb(0, 255, 0)', }; // Create a function for setting a css variable value const cssRoot = document.querySelector(':root'); const setCssVariable = (variable, value) => cssRoot.style.setProperty(variable, value); // Create our settings GUI const SettingsPane = new Tweakpane.Pane({ title: 'Settings', expanded: true, }); // Setup settings const Setup = SettingsPane.addFolder({ title: 'Setup', expanded: true, }); Setup.addInput(Settings, 'username').on('change', (ev) => { document.querySelector('.username').innerText = ev.value; }); // Display settings const Display = SettingsPane.addFolder({ title: 'Display', expanded: true, }); Display.addInput(Settings, 'background_color').on('change', (ev) => { setCssVariable('--background-color', ev.value); }); Display.addInput(Settings, 'message_color').on('change', (ev) => { setCssVariable('--message-color', ev.value); }); Display.addInput(Settings, 'username_color').on('change', (ev) => { setCssVariable('--username-color', ev.value); }); ``` I could manually update the css variables, but it's just more code duplication. _(and could be a lot of code depending on how many settings you have)_ ```js document.querySelector('.username').innerText = Settings.username; setCssVariable('--background-color', Settings.background_color); setCssVariable('--message-color', Settings.message_color); setCssVariable('--username-color', Settings.username_color); ``` compared with just 1 call to: ```js SettingsPane.refresh(true); ```
cocopon commented 2 years ago

The former one looks better to me. I don't think that shorter code is always appropriate.

The standard change event isn't fired for programmatic changes. For example:


const elem = document.createElement('input');
document.body.appendChild(elem);
elem.addEventListener('change', (ev) => document.title = ev.target.value);

// This doesn't invoke change event
elem.value = 'foobar';

Ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event

cocopon commented 2 years ago

Feel free to reopen the issue if you have any updates.

scarletsky commented 2 years ago

I have the same question. My use case is like this:

When user click currentView radio button group, the root pane will destroy the old folder and create a new folder then update its value. image

The code is here:

const state = {
    currentViewIndex: 0,
    currentViewPane: null,
};

const createViewPane = (view) => {
    if (state.currentViewPane) {
        rootPane.remove(state.currentViewPane);
        state.currentViewPane = null;
    }
    const folder = state.currentViewPane = rootPane.addFolder({ title: view.name });
};

const currentViewIndexPane = rootPane.addInput(state, 'currentViewIndex', {
    label: 'currentView',
    view: 'radiogrid',
    groupName: 'currentViewIndex',
    size: [3, 1],
    cells: (x, y) => ({
        title: `view-${y * 3 + x}`,
        value: y * 3 + x,
    }),
}).on('change', (ev) => {
    const view = views[ev.value];
    view.setState({ enabled: true });
    createViewPane(view);
});

But I need to programmatically trigger change event once the pane created, otherwise it will miss the folder like this: image

And now I found a solution:

currentViewIndexPane.controller_.valueController.value.setRawValue(state.currentViewIndex, {
    forceEmit: true // NOTE: This is important, without this it will NOT work
});

Although it works, it is too boring to call the setRawValue API of a BladeApi instance...

I am not sure if this is a correct way to trigger change event programmatically... ?

cocopon commented 2 years ago

I think that event handlers are literally for handling events. It's a bit strange to me to emit a change event manually although there is no change. If you want to call it manually, you can separate it from the handler like this:

function applyValue(value) {
  ...
}

pane.addInput(PARAMS, 'key').on('change', (ev) => {
  applyValue(ev.value);
});

// Update manually
applyValue(PARAMS.key);
scarletsky commented 2 years ago

@cocopon Thank you for the quick reply. Your solution is another way to write the callback. I think my use case might look more like updating UI programatically, and there is no example on the document.

By default, Tweakpane doesn't detect changes of bound parameters.

The Misc page just tell us how to refresh the pane, but it does not tell us how to update the bound parameters.

cocopon commented 2 years ago

You mean that adding more examples like this case can be helpful, right?

scarletsky commented 2 years ago

You mean that adding more examples like this case can be helpful, right?

The example above is no need to call pane.refresh()... I think the document should add more examples about WHEN and WHY should we call pane.refresh.

cocopon commented 2 years ago

Hmm, it's difficult to balance conciseness and concreteness. I'm giving priority to former one for readability, at least for now.

The document says:

By default, Tweakpane doesn't detect changes of bound parameters. Use refresh() to force-update all input/monitor components.

In other words, you can use refresh()...

WHEN: You want to force-update all bindings WHY: Tweakpane doesn't detect changes of bound parameters

scarletsky commented 2 years ago

WHEN: You want to force-update all bindings

When user interact with UI (the pane), the binding values should be updated correctly, and you don't need to call refresh() manually.

In other words, only when you update the value programatically, you should call refresh(). This is same as those popular two-way bindings framework, like Angular, Vue etc.

In my opinion, the document now miss how to update the value programatically. Update value from UI is not enough in complex scenario. 😄

cocopon commented 2 years ago

Okay, I'll try to improve this section later. Pull requests are also welcome.

scarletsky commented 2 years ago

I think the setRawValue API should be added to the document.

If not, add another simple API (like Controller#setValue in dat.gui) will be supper useful to update value programatically.

I am not familiar with the source code of tweakpane, what I can do is just update the document examples to tell guys how to use setRawValue.

I will prefer to add another setValue API, the solution in #418 is too annoying for me...

cocopon commented 2 years ago

Is there a reason you don't change parameter values directly? Tweakpane just provides a UI for changing parameter values. Changing them outside of the pane would be out of scope of the library.

scarletsky commented 2 years ago

A little example here

const state = { A: 0, B: 1 };

If A has relation with B, it means when A changed, B should be changed too. In this case we should update value programatically right ?

cocopon commented 2 years ago

Right. But in this case, the outside should take responsibility for managing a state, not the pane.

scarletsky commented 2 years ago

Of cause we should manage the state outside. The question is when the state changed, the pane not change at the same time.

cocopon commented 2 years ago

Then you can use refresh() to apply changes to bindings, or this pattern for more complex situation.

I still don't understand the validity of setValue-like API. What you want to do is changing parameter values, not setting a value to the pane. Parameter values are the leading role and Tweakpane is the supporting role.

scarletsky commented 2 years ago

Hmm, now I understand how you design tweakpane (or how tweakpane works). Tweakpane is a data-driven UI, providing one-way data binging. It is more like React than Angular and Vue. If so, setValue API may not need (although sometimes it is convenient).

Just add more examples for refresh API in the document may help a lot. 😄

donmccurdy commented 1 year ago

+1 for a bit more detail or examples on the 'refresh manually' part of the documentation. I recently ran into a surprise finding that when I call .refresh(), the change events of all affected controls are fired... I guess that makes sense in retrospect, but it wasn't what I expected, and caused a subtle problem for my application.

aalavandhaann commented 1 year ago

I am not sure if you are still looking for a solution to this problem. I ran into (recursive) problems due to updating of values programmatically adn wanted to refresh the controls. Since, the variable that the inputbindingapi controls is also being listened to with an event listener this was making my life literally a nightmare. However, I was able to solve it like this.

function onChanged(evt: any): void{
   //control.refresh();//Ran into maximum call stack exceeded issue due to recursion
   // Instead I did the below
  control.disabled = true;
  control.refresh();//This will not trigger the on change event and work around the recursion issue. 
  control.disabled = false;
}
let variable: Listenable = new Listenable({value: 5});
let control: InputBindingApi<number, number> = pane.addInput(variable, 'value', {value: variable.value});

variable.addEventListener('changed', onChanged);