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.

[finetjul]

I think the example is fine. It does what InteractorStyleMPRSlice consumers can do.
We want to have InteractorStyleMPRSlice to behave correctly.

To me, the problem is that InteractorStyleMPRSlice is triggering an event (and let consumers react) while being in an inconsistent state. We should not trigger such event when being in an inconsistent state.

[Jo-Byr]

To me, the problem is that InteractorStyleMPRSlice is triggering an event (and let consumers react) while being in an inconsistent state

I disagree. For me the fix is to

1. Add a if (slice === publicAPI.getSlice()) { return true } guard at the start of setSlice

2. Fix the NaN issue in the example.

The NaN issue is a bit tedious to understand. It comes from the fact that we don't explicitely initialize the slider range at the initialization which makes it so that the first call to updateUI does the initialization with the value of the interactor style which by default are min = 0, max = 0.

lil-gui sliders have a _step attribute that's initialized to (max - min) / 1000 when setting min and max for the first time and from then can only be modified manually.

lil-gui has no guard against incoherent min and max. We can have min > max and then the step is negative (i.e. up-arrow decreases the slider value), but for min == max, we then have _step = 0 which makes it so that Math.round( value / this._step ) * this._step;, which is called for sanitizing the value in sliceCtrl.setValue, returns NaN.

So I see 2 solutions:

1. Initialize min and max to 0 and 1 at the start

2. Update step when changing min and max

I prefer 2 as it maintains a step to 0.1% of the range, rather than the (1-0) / 1000.

[finetjul]

1. and 2. are good and should be done. (I also prefer 2.2 over 2.1)

But it is not sufficient.

Say you call:

 mprSlice.setSlice(4);
 ...
 mprSlice.setSlice(5);
    // In the setSlice(5) function call, `camera.setPosition(...newPos);` is being called, 
    // this triggers an event and the application/example is reacting to it
    // and it calls `mprSlice.setSlice(5)` again (nested call). 
    // `if (slice === publicAPI.getSlice()) { return false; }` won't do an early return
    // because `publicAPI.getSlice()` will still return 4 at that time because 
    // `camera.setFocalPoint(...)` has not yet been called.

[Jo-Byr]

So replacing the double set by

camera.set({
  position: newPos,
  focalPoint: slicePoint,
});

?

[finetjul]

So replacing the double set by

camera.set({
  position: newPos,
  focalPoint: slicePoint,
});

?

It would have to be camera.set({...}, noFunction=true) to be different from camera.setPosition();camera.setFocalPoint(). Problem is, it wouldn't call camera.computeDistance() like setPosition or setFocalPoint() would do. Nor would it call camera.modified().

I guess we could create a convenient camera.setPositionAndFocalPoint(x, y, z, x, y, z)

[Jo-Byr]

I guess we could create a convenient camera.setPositionAndFocalPoint(x, y, z, x, y, z)

Should I create a separate PR for that ?

[finetjul]

I guess we could create a convenient camera.setPositionAndFocalPoint(x, y, z, x, y, z)

Should I create a separate PR for that ?

no, it's part of the fix. It can be another commit in the same PR.