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
40 stars 55 forks source link

measure() override returns incomplete data #219

Open mkahvi opened 2 years ago

mkahvi commented 2 years ago

Ruler.measure() normally returns data like this:
normal-data

But with Drag Ruler enabled, it instead returns this:
dragruler-data

This can and will break compatibility with anything else that relies on measure()'s return value.

mkahvi commented 2 years ago

The ray distance also always seems to be 100 no matter how far you measure. This was some other problem with my test case.

manuelVo commented 2 years ago

Interesting. I suppose this changed at some point in foundry without me noticing. Thanks for telling me about this.

manuelVo commented 2 years ago

What is your module doing with the return value of measure? As part of the v10 port, Drag Rulers pathfinding will become async to prevent foundry freezes when long paths are being calculated. Of course this has the disadvantage that the measurement results won't be available immediately. I'm considering to build the measure function like this: If a measurement is performed that can be evaluated immediately (meaning the pathfinder isn't being used) the function will return what you'd expect it to. If pathfinding is used, then the measure function will instead return a promise that will yield the expected return value once it's resolved. What do you think?

mkahvi commented 2 years ago

Measure info module: https://foundryvtt.com/packages/koboldworks-measure-info

Tho if you make it async when the base Foundry functionality is not, it's going to break even more things. If you do that however, you better add very noticeable warning about it in the package page and in its description.

I can just mark my module as incompatible with yours, of course.

manuelVo commented 2 years ago

I'd really love to find a way to have compatibility, which is why I'm asking for feedback. The other option I see would be to just return an empty result (meaning pretending the measured path has 0 length) if the pathfinder is running. That way other modules that rightfully expect measure to be sync would at least not break - though they couldn't receive a proper result, either. I could add some kind of hook that's being called once pathfinding is done so modules that are aware of the pathfinding-asyncness could update accordingly...

mkahvi commented 2 years ago

Fake result would still make it essentially break compatibility, since although it won't make my module produce NaN display, it will still not work at all, so the conflict persists even if it looks like it doesn't.

In other words, it would make it harder to determine what is breaking, but not solve the issue.

manuelVo commented 2 years ago

I guess in that case I'll go for the first idea. That way other module authors have the chance of building compatibility, while incompatible modules will break in an obvious way. It's not ideal, but leaving pathfinding as a sync calculation really isn't workable for larger scenes