pixi-viewport

pixi-viewport copied to clipboard

Calls to ~setZoom~ functions that change scale (setZoom, fit*** etc.) do not emit zoomed and zoomed-end events. For me, I expect zoom events to fire regardless of what triggered the zoom, but this may be a BC break for apps that don't expect it. So, how to fix this properly?

My "workaround" looks like this:

const zoomingFunctions = ['setZoom', 'fitWidth', 'fitHeight', 'fitWorld', 'fit'];
  zoomingFunctions.forEach(zoomingFunctionName => {
    const oldFunction = viewport[zoomingFunctionName].bind(viewport);
    viewport[zoomingFunctionName] = (...args) => {
      const prevX = viewport.scale.x;
      const prevY = viewport.scale.y;
      const result = oldFunction(...args);
      if (prevX !== viewport.scale.x || prevY !== viewport.scale.y) {
        viewport.zooming = true;
        viewport.emit('zoomed', {viewport, type: 'manual'});
      }
      return result;
    };
  });

Not firing the zoom event when manually triggering was a design decision. I see arguments both ways, but it's harder to know when to skip the event than it is to add it manually (at least from my perspective).

Your hack in general looks okay. You would need to test it to make sure it performs as expected, but I don't see why it wouldn't. Depending on how often you're calling these functions, it may be easier to add the emit during your local uses. For example, call

viewport.fit()
viewport.emit('zoomed', { viewport, type: 'manual' }

Again, depends how often you're doing this and how much work it is to remember to cover all cases.

You also have the option of forking pixi-viewport and adding the emit line for these functions in your fork. I don't foresee any merge issues down the road when you want to pull the latest version.

Good luck!

Yes, thanks for answer, that are possible solutions for me. I just thought this may be useful for others - maybe adding a special event like zoom-change, zoom-manual or so that will be triggered on [any/only manual] zoom changes would be helpful. I realized merging manual zoom changes with UI zoom event isn't the best idea, since modifying the zoom from the event handler may easily lead to infinite loops.

Such event should be used with caution only with handlers that just react to a zoom change and do not further modify the zoom (like clamp-zoom), but is useful in complex apps that need to adjust something on any zoom change regardless whether it comes from UI or app.

I'm willing to make a simple PR for this if you want.

I like the idea of a differently named event. Something like zoom-manual would work. If you submit a PR I can get it merged and published. Thanks!

@davidfig today I looked onto this issue again and noticed that you already emit zoomed event when it is programmatically changed, namely type=clamp-zoom and type=ensureVisible. I think if we added an event like zoom-manual, it would make the behaviour weirdly inconsistent. Especially for clamp-zoom: for example I start with zoom=5, clamped to [1, 5]. Then I call setZoom(10). What to emit now? both zoomed#type=clamp-zoom and zoomed-manual? or only one of the events, or none? I think this doesn't have an intuitive solution.

So I'd reconsider if we shouldn't emit zoomed consistently on all zoom changes, initiated by UI or app. But then we should still figure out what to do with clamp-zoom and ensureVisible. I think they should ideally both be removed, because:

• clamp-zoom is actually not a zooming event, it is a constraint of any action that changes zoom. So if one spins the mouse wheel and clamp-zoom restricts it, zoomed should not be fired since no actual zooming was done; if one calls setZoom and it is clamped to a lower value, it should emit something like zoomed#set-zoom regardless if clamping was used on the result.
• ensureVisible calls fit, so if we rewrite fit to trigger zoomed event, it would trigger two zoomed events this way.

What do you think?