manuelVo / foundryvtt-drag-ruler

A Foundry VTT module that shows a ruler when dragging tokens so you can see how far you've dragged them
MIT License
39 stars 50 forks source link

V12 compatibility #329

Closed TPNils closed 1 week ago

TPNils commented 3 weeks ago

Personal rambelings

First off, thanks for all the hard work you have put into this module. I noticed in the issue list #326 you are currently very busy and I really wanted to use Drag Ruler in a new campaign that we will start soon in V12, so I thought: I can develop modules! Why not have a look?

What you probably care about

I managed to get Drag Ruler to work on both V11 and V12, I did some test as GM, as player, with and without routingLib & the dnd5e extension and things seem to be working but has not been "field tested" yet.

Cempres commented 3 weeks ago

Does the original author have to confirm the commits for us to use this?

TPNils commented 3 weeks ago

I suppose it doesn't have to be. I just made a beta release for you. You can manually install it with the module.json provided in the release.

I would like to kindly ask to not bother the original author with issues on V12 since this is an unofficial and experimental release

Manual installation: image

Cempres commented 3 weeks ago

Appreciate your contribution to this, hopefully your branch helps the creator to create a stable release!

n0q commented 1 week ago

Nice one! This absolutely gets things going, though it does seem to crash at times, resulting in no label being printed for the ruler at all. I haven't investigated too deeply yet to describe how to reproduce this, but I will drill down further later. I'm seeing this exhibited on a gridless game.

image

manuelVo commented 1 week ago

I finally got around to testing and reviewing the changes. Unfortunately, the canvas doesn't load at all on my system. This is the error that gets spammed.

grafik

Since people seem to be successfully using your patch, I assume it cannot be inherently broken. Any idea what might be going on?

TPNils commented 1 week ago

I finally got around to testing and reviewing the changes. Unfortunately, the canvas doesn't load at all on my system. This is the error that gets spammed.

grafik

Since people seem to be successfully using your patch, I assume it cannot be inherently broken. Any idea what might be going on?

I'm afraid I have not encountered this error before :/ Maybe WebGL was unable to load would be my best guess? Have you tried loading foundry without any modules?

I can trace the error to PIXI, maybe they know what it means https://github.com/pixijs/pixijs/blob/9104ebd911ac97cd58928d476b588061fa2ad680/packages/core/src/geometry/GeometrySystem.ts#L229

TPNils commented 1 week ago

Nice one! This absolutely gets things going, though it does seem to crash at times, resulting in no label being printed for the ruler at all. I haven't investigated too deeply yet to describe how to reproduce this, but I will drill down further later. I'm seeing this exhibited on a gridless game.

image

I'm afraid I seem to be unable to reproduce this error on a gridless system with foundry version 12.330

n0q commented 1 week ago

I'm afraid I seem to be unable to reproduce this error on a gridless system with foundry version 12.330

It might involve the speed provider. I'll dig further.

MIBIB40000 commented 1 week ago

Hi! I'm trying to use it with wfrp4e, but have a duplicate of token when moving, then got an error in console. image Any ideas, is it package or system problem?

Foundry v12 328

TPNils commented 1 week ago

Hi! I'm trying to use it with wfrp4e, but have a duplicate of token when moving, then got an error in console. image Any ideas, is it package or system problem?

Foundry v12 328

wfrp4e does not officially support V12 and the error does happen in the wfrp4e code, which leads me to believe this might be related to wfrp4e, although I am not familiar with the system

MIBIB40000 commented 1 week ago

wfrp4e does not officially support V12 and the error does happen in the wfrp4e code, which leads me to believe this might be related to wfrp4e, although I am not familiar with the system

Thank you for clarification! Sorry to bother you with this.

UPD: my friend found a solution for wfrp4e with v12 foundry - we comment out 20088 row with 'this.parent.diagonalRule;' and module working now. I don't know what bugs this will lead to in future, but it might be useful for someone.

n0q commented 1 week ago

In further testing, I've found a much bigger issue than what I initially suggested. From testing on gridless maps, I'm finding that ruler.rulerOffset is probably double-correcting. L142 of main.js:

ruler.rulerOffset = {
    x: entityCenter.x - event.interactionData.origin.x,
    y: entityCenter.y - event.interactionData.origin.y,
};
This seems meant to counter where the user grabbed the token and force it back to center. In v11, it prevents the issue that it now causes in v12. See images below. Gridless scene, with lines showing five foot marks. Deliberately grabbed an image off center, trying to drag it five feet in the -x direction: Pre-Move Post-Move

I'm pretty sure this happens constantly, but it's only visible on gridless and hex grids because square grids are orthogonal wrt the canvas, so the offsets don't cause an apparent issue. I can fix this via hack:

    const isV11 = game.release.generation === 11;
    ruler.rulerOffset = {
        x: isV11 ? entityCenter.x - event.interactionData.origin.x : 0,
        y: isV11 ? entityCenter.y - event.interactionData.origin.y : 0,
    };
n0q commented 1 week ago

Also note this PR breaks on hex grids. I've submitted a pr downstream to TPNils to correct both issues.

n0q commented 1 week ago

I finally got around to testing and reviewing the changes. Unfortunately, the canvas doesn't load at all on my system. This is the error that gets spammed.

grafik

Since people seem to be successfully using your patch, I assume it cannot be inherently broken. Any idea what might be going on?

Silly question, but you did update your game system to v12, right? Someone reported that issue on reddit and their problem was being on the v11 version of D&D 5e.

manuelVo commented 1 week ago

Not that silly of a question. You nailed it @n0q. This PR is now merged as 0808f17 and I also took the liberty to pull in your PR in TPNils' fork as 0e2ab35. I've also gotten rid of most of the deprecation warnings - though a few remain (which in Drag Ruler unfortunately always means the console will be absolutely spammed). Release of those changes is imminent. Thanks your work and for your patience with me, guys! :)