chore(InteractorStyleMPRSlice): fix recursion error in example

[Jo-Byr]

Context

InteractorStyleMPRSlice example fails due to a recursion error.

Results

The error is fixed so the example works.

Changes

Added a difference threshold on the interactor Stume slice update to avoid recursion error

PR and Code Checklist

• semantic-release commit messages
• Run npm run reformat to have correctly formatted code

[finetjul]

It should be fixed in InteractorStyleMPRSlice instead of the example.

[finetjul]

I'm glad this fixes the example.
However, I think it does not fix the root of the problem.
The InteractorStyleMPRSlice is temporarilly notifying observers of an invalid "slice".
The problem is that it triggers a modify event between line 174 and line 175. When the state is unstable.
A proper fix should be to prevent such notification while the camera is being updated.

[Jo-Byr]

A proper fix should be to prevent such notification while the camera is being updated.

I moved the first updateUI call and the istyle.onModified callback to after the first render call.

[finetjul]

A proper fix should be to prevent such notification while the camera is being updated.
I moved the first updateUI call and the istyle.onModified callback to after the first render call.

While you may have fixed the example. You have not fixed all the consumers of InteractorStyleMPRSlice. It would be better to fix InteractorStyleMPRSlice in the first place (rather than the example (i.e. a consumer of InteractorStyleMPRSlice)).

[Jo-Byr]

While you may have fixed the example. You have not fixed all the consumers of InteractorStyleMPRSlice. It would be better to fix InteractorStyleMPRSlice in the first place (rather than the example (i.e. a consumer of InteractorStyleMPRSlice)).

I did this too. The block

if (publicAPI.getSlice() === slice) {
  return false;
}

In InteractorStyleMPRSlice's setSlice is responsible for that.

The modification in the example is to fix this problem you mentioned:

A proper fix should be to prevent such notification while the camera is being updated.

[finetjul]

From what I remember, InteractorStyleMPRSlice.setSlice() calls camera.setPosition(...newPos); and then camera.setFocalPoint(...slicePoint);
In this context, when camera.setPosition() is called, it triggers a camera ModifiedEvent, thtat calls ModifiedEvent on InteractorStyleMPRSlice. If an observer queries InteractorStyleMPRSlice.getSlice() when InteractorStyleMPRSlice is modified, it may get a wrong slice number. (because camera.setFocalPoint() has not yet been called with the correct value)

[Jo-Byr]

From what I remember, InteractorStyleMPRSlice.setSlice() calls camera.setPosition(...newPos); and then camera.setFocalPoint(...slicePoint); In this context, when camera.setPosition() is called, it triggers a camera ModifiedEvent, thtat calls ModifiedEvent on InteractorStyleMPRSlice. If an observer queries InteractorStyleMPRSlice.getSlice() when InteractorStyleMPRSlice is modified, it may get a wrong slice number. (because camera.setFocalPoint() has not yet been called with the correct value)

The issue actually comes from getSlice not returning the exact same value as setSlice by a few digits, which triggers a set on the controller in the UI, which in turn retriggers setSlice which will again be slightly off.

We just need to block the UI update in updateUI:

let blockUIUpdate = false;

const sliceCtrl = gui
  .add(params, 'Slice')
  .name('Slice')
  .onChange((value) => {
    if (!blockUIUpdate) { <--
      istyle.setSlice(Number(value));
      renderWindow.render();
    }
  });

function updateUI() {
  const range = istyle.getSliceRange();
  const slice = istyle.getSlice();
  const normal = istyle.getSliceNormal();

  sliceCtrl.min(range[0]);
  sliceCtrl.max(range[1]);
  blockUIUpdate = true; <--
  sliceCtrl.setValue(slice);
  blockUIUpdate = false; <--
  ...
}

Do you agree ?

[finetjul]

Do you agree ?

No, I disagree :-) It should not be fixed in the example but in InteractorStyleMPRSlice.

[Jo-Byr]

No, I disagree :-) It should not be fixed in the example but in InteractorStyleMPRSlice.

It seems just adding if (slice === publicAPI.getSlice() { return true; } at the top of setSlice is enough to fix the recursion that happened prior to the scene being properly initialized.

[finetjul]

No, I disagree :-) It should not be fixed in the example but in InteractorStyleMPRSlice.

It seems just adding if (slice === publicAPI.getSlice() { return true; } at the top of setSlice is enough to fix the recursion that happened prior to the scene being properly initialized.

It is enough for the example, but it may not be enough for other consumers of InteractorStyleMPRSlice.

[Jo-Byr]

It is enough for the example, but it may not be enough for other consumers of InteractorStyleMPRSlice.

I think that fundamentally, it's an example problem. It's the example that implements a listener that's also a setter, which of course can create a recursion error if we decide to display the data differently from what we get.

But with the setter guard mentionned, there's no more erorr in the example. What else would you want to add ?

There just seems to be another bug due to the calls to sliceCtrl.min/max in updateUI that cause the value to get set to NaN.