fantasycalendar / FoundryVTT-Sequencer

This module implements a basic pipeline that can be used for managing the flow of a set of functions, effects, sounds, and macros.
Other
47 stars 25 forks source link

Improve compatibility with Walled Templates module #78

Closed caewok closed 2 years ago

caewok commented 2 years ago

This is part bug but mostly a feature request. I'm the author of the Walled Templates module. I have an issue with compatibility between Sequencer and Walled Templates that I think I need your help to resolve.

Basically, when Walled Templates draws a template, it uses ClockwiseSweep to determine if the template intersects walls, and uses that resulting PIXI.Polygon in lieu of the template shape. This results in the error I quote below, but only for square/cube templates.

To Reproduce Steps to reproduce the behavior:

  1. Install Walled Templates, Sequencer, Automated Animations, JB2A (the last two are probably optional but easier to test with AA)
  2. Make sure "Default to Walled Templates" is set in Walled Templates so the template will be modified by walls.
  3. Cast Fireball or your favorite template spell. As a circle, it should work. Change to square template and cast again. Should throw the error

Expected behavior Ideally, when Sequencer encounters a PIXI.Polygon as the shape, it would react accordingly. Bonus points if you can figure out how to constrain an animation inside an arbitrary closed polygon, but not really necessary to fix the error.

Possible changes My suspicion is that Sequencer is going awry at get_object_position in canvas-lib.js. It looks like for rectangles, it may be trying to adjust the position by the width and height of the rectangle—without a rectangle, that is going to fail.

It looks like by default, Sequencer otherwise falls back on the {x,y} location property for the shapewhich in this case would be the origin of the template and all is good. Probably why this works for other shape types. While I could re-define the template's data.t property to be something other than "rect" for the rectangle template, I am loathe to do that because I cannot predict what other systems and modules are doing with that information. The template is still indeed a "rect", just one being blocked by a wall.

If I am right in my suspicion, the fix here might be very simple. Just harden the rectangle test to avoid shape and width:

        if (measure) {
            if (obj.data.t === "cone" || obj.data.t === "ray") {
                pos.x = obj.ray.B.x;
                pos.y = obj.ray.B.y;
            }
        }
        if (obj.data.t === "rect") {
            pos.x = obj.x
            pos.y = obj.y

            if(!exact && obj.shape.width && obj.shape.height){
                pos.x += Math.abs(obj.shape.width / 2) + (obj.shape.x)
                pos.y += Math.abs(obj.shape.height / 2) + (obj.shape.y)
            }
        }

But there might be more going on here. Happy to help if I can!

Screenshots https://user-images.githubusercontent.com/1267134/171737009-6a4a48c1-c875-464b-b1ea-481a7de347c3.mov

Desktop (please complete the following information): Not sure what the original person who filed the issue was running, but I am running:

Additional context Error message:

lib.js:403 Uncaught (in promise) Error: Sequencer | Module: Automated Animations | Effect | play - Could not determine where to play the effect! [Detected 1 package: sequencer] at Module.custom_error (lib.js:403:12) at Proxy._customError (sequencer.js:182:20) at Proxy._expressWarnings (effect.js:846:33) at Proxy.run (effect.js:855:14) at section.js:169:30

Haxxer commented 2 years ago

I use get_object_position to get the center point of the rectangle, if I were to do what you suggest and base it off the polygon, then would that not result in a position that is not at the center of the template, but rather some other arbitrary point?

caewok commented 2 years ago

True. What if I calculated the centroid of the polygon and passed through those coordinates in the PIXI.Polygon shape? We would need some agreement on what that point should be called. Right now, the MeasuredTemplate object has an x,y property already—that is the origin of the template. That is obviously incorrect for your purposes.

I could add an x,y to the PIXI.Polygon shape, so if template is a MeasuredTemplate, and shape is a PIXI.Polygon, it would look like template.shape.x and template.shape.y. That would be consistent with a circle shape's use of x,y as the center point. You might need to check for whether the shape is a PIXI.Polygon, however, because PIXI.Rectangle annoyingly uses x,y to mean the top left corner and not the center. Same I guess for lines (ray) and cones, neither of which use x,y to mean the center.

What do you think, would adding a centroid property help here?

Haxxer commented 2 years ago

It's less about the center point of the polygon, and more about the original center point of the template itself

Haxxer commented 2 years ago

Imagine this, if someone placed a template with a certain dimensions, and expect the effect to fit inside those dimensions, even if you cut away parts of the template, the effect still ought to be placed in the center of the template.

caewok commented 2 years ago

Okay, thanks for the explanation; very helpful! As all AA is expecting here is to be able to determine the center of the original template, I added an easy fix: I just added the x,y, width, and height properties AA needs to make that calculation. So the error is resolved.

Long term, let me know if any other errors or issues crop up with Walled Templates and I will try to address them if I can.

Haxxer commented 2 years ago

Thank you! That's probably the path of least resistance! :)