KayelGee / select-tool-everywhere

MIT License
3 stars 12 forks source link

Fix for canvas crashes when dragging lights #6

Closed Aedif closed 1 year ago

Aedif commented 1 year ago

Fix for: https://github.com/KayelGee/select-tool-everywhere/issues/5

Light controls are re-enabled if libWrapper is active. Patches AmbientLight._onDragLeftCancel to defer updates and prevent race conditions when dragging multiple lights.

KayelGee commented 1 year ago

I'd rather add libwrapper as a dependency instead of checking if libwrapper is there

Aedif commented 1 year ago

To avoid the libWrapper dependency something like this could also work:

  function lightDragLeftCancel(event) {
    Object.getPrototypeOf(AmbientLight).prototype._onDragLeftCancel.call(this, event);
    this.updateSource({ defer: true });
  }

  Hooks.on("controlAmbientLight", (light) => {
    if (light.mouseInteractionManager?.callbacks) {
      light.mouseInteractionManager.callbacks.dragLeftCancel = lightDragLeftCancel.bind(light);
    }
  });
Aedif commented 1 year ago

I've added another commit for you to consider. I noticed a while ago that control borders get hidden when Sounds, Templates, and Lights are dragged around. Moving the .controlled check to a refresh hook would make the borders survive through these sort of operations.

KayelGee commented 1 year ago

To avoid the libWrapper dependency something like this could also work:

  function lightDragLeftCancel(event) {
    Object.getPrototypeOf(AmbientLight).prototype._onDragLeftCancel.call(this, event);
    this.updateSource({ defer: true });
  }

  Hooks.on("controlAmbientLight", (light) => {
    if (light.mouseInteractionManager?.callbacks) {
      light.mouseInteractionManager.callbacks.dragLeftCancel = lightDragLeftCancel.bind(light);
    }
  });

Since dependencies get auto installed I think that it's ok to have a library as a dependency, especially as overriding core functionality is so error prone when multiple modules do it. I think it's better to have libWrapper notify the user in case some modules are incompatible because they override the same function.

Aedif commented 1 year ago

Alright I'll change it back to a wrapper. Also I re-enabled Note controls using the module. While v10 does provide a select tool for Notes it is actually completely non-functional. I submitted a bug report for it, but I think they're more likely to remove the tool rather than fix it...

Also there is quirk with Notes in particular that necessitate this ugly thing:

  Hooks.on("drawNote", (note) => {
    setTimeout(() => {
      SelectToolEverywhere.placeableRefresh(note);
    }, 10);
  });

Annoyingly unlike Lights, Sounds, and Templates, the Notes keep losing their borders after being moved... Note refresh hook is called before ControlIcon has been drawn (after which the visibility is set to false), and this being the last hook called after the Note has been moved leaves us with a bit of a problem...

The only solution I could come up with is slightly delaying the re-enabling of the border. Perceptually makes no difference to the user, but it's not the prettiest workaround.

KayelGee commented 1 year ago

I didn't realize the note select tool was non functionial.

KayelGee commented 1 year ago

I've seen no problems and have merged your changes

KayelGee commented 1 year ago

Also: Thank you!