zeel01 / scene-tiler

2 stars 1 forks source link

if (!canvas.grid.hitArea.contains(x, y)) x = y = 0 in getTilePos function is too restrictive. #4

Open ggagnon76 opened 3 years ago

ggagnon76 commented 3 years ago

Hello Zeel!

First, I want to thank you for making the API and improving the usability of your module.

Now to get down to my issue. When working on my proof of concept, I ran in to an issue. It took me a while to figure out what was happening, but the exercise was fruitful and may also point out some of the ways people might get stuck trying to use your module.

My setup:

Main scene is 1200 x 1200 (a square). My compendium scene I want to tile is 800 x 1200 (rectangular)

In my module, I need to be able to position my compendium scene in specific orientations, ie: it can be rotated 90 degrees, 180 degrees or 270 degrees. When I rotate them like this, I want the tile to butt up against the outside of the main scene, with a specific side always facing the center of my main scene. Words may not be clear enough, so here's a pic:

SceneTiler Example Rotations

The red square is the main scene, 1200 x 1200. The green square is the compendium scene that becomes the tile. The purple dot (in the 0 degrees quadrant) is the tile X, Y origin. The yellow dot in each quadrant is the tile center, which is also the center of rotation.

If I drop my scene at coordinate 0, 0 with zero rotation, everything works fine. If I try to drop my scene with a 90 degree rotation as shown in the pic above, it turns out to be impossible with your module. I'll explain why, but first here's a pic:

SceneTiler example positioning

What you see here is the end goal, my 800x1200 compendium scene rotated so it becomes 1200x800, and positioned at the top of my main scene. In transparent, I've shown the position of the compendium scene pre-rotation, with the center (yellow dot) coinciding with the center of my desired position. As you can see, the origin coordinate (purple dot) of the tile pre-rotation is outside of my scene. The coordinate is specifically {x: 200, y: -200}.

And that is the problem. In your getTilePos function, you have the following line of code:

if (!canvas.grid.hitArea.contains(x, y)) x = y = 0

This basically says, if the origin coordinate of the tile is not in the scene, then set the origin to {x: 0, y: 0}.

I'm pretty sure I understand your intent with this line of code; to prevent bad code code from dropping tiles completely outside of the scene and thus causing confusion. Bit it is too restrictive. I personally think I should be allowed to drop a tile even if only a portion of that tile is visible in the final scene. This is entirely possible if the tile is dropped on the right or bottom sides. But it is impossible to do on the left or top sides because the tile origin (top and left) has to be inside the scene.

As an aside, because I was figuring this out, I ended up trying several different things that can easily be misunderstood or not intuitive to everyone. In the following:

async create(scene, { x, y, rotation, populate, centered })

Documentation says x and y are the position of the center of the tile, and they are optional. If centered is true (also optional), then the tile position is shifted to be relative to the center of the tile.

Having x, y and centered = true as arguments in that function, as defined in the documentation, confuses me. I think the x and y represent the top left corner of the tile, not the center? And if you have centered true, then you're telling your module you want the x and y coordinates provided to be the center of the tile, not the top left. As documented, it doesn't make sense to me.

Also, since I think I'm right that the x and y represent to top left coordinates, unless centered is true, it would help if you documented to explain if the x and y coordinates apply pre rotation, or post rotation. As you can see in my examples, a rectangular compendium scene will produce a rectangular tile, and the x,y coordinates change based on rotation...

zeel01 commented 3 years ago

In your getTilePos function, you have the following line of code:

if (!canvas.grid.hitArea.contains(x, y)) x = y = 0

This basically says, if the origin coordinate of the tile is not in the scene, then set the origin to {x: 0, y: 0}.

Alright, so this I see the issue with. This check was created to prevent dragging and dropping a tile to an invalid location, but for an API consumer it makes much more sense for this check not to happen. It's not really to check bad code but to check users making mistakes, it's just incidentally also effecting the API but I can fix that easily enough.

Documentation says x and y are the position of the center of the tile, and they are optional. If centered is true (also optional), then the tile position is shifted to be relative to the center of the tile.

Ah, there was an error in the documentation. The x and y should not say "the center of the tile" anymore, since I added the centered option. The behavior is that if x and y are defined, and centered = false the coordinates are the upper-left corner. If centered = true the coordinates are the center of the tile. For most API users, the centered option should be omitted, it's another thing that exists primarily to facilitate the drag-and-drop workflow.

Also, since I think I'm right that the x and y represent to top left coordinates, unless centered is true, it would help if you documented to explain if the x and y coordinates apply pre rotation, or post rotation. As you can see in my examples, a rectangular compendium scene will produce a rectangular tile, and the x,y coordinates change based on rotation...

So this is one of the reasons that originally I was using the center coordinate rather than upper-left, it doesn't change if you rotate the tile which is convenient. I'm actually not entirely sure if the coordinate is the pre or post rotation coordinate, as tile placement/rotation is being handled by Foundry core.

...Did some investigation: The coordinate doesn't change when you rotate the tile, so it's a pre-rotation coordinate. This is unfortunately just how Foundry does its thing, so calculating the correct offset would be necessary to get the placement right.

ggagnon76 commented 3 years ago

Other than the hit area check, nothing else needs fixing besides documentation, which I'm aware you're already doing.

As for me, I do want to use the centered = true via the API, so I don't find it useless as an API user. It's a good option to have, especially if dealing with rotations.

zeel01 commented 3 years ago

Still needs resolved.