search

CarlosFdez / world-explorer

Foundry VTT Module for manual hexcrawl revealing
17 stars 5 forks source link

feat: persist explored areas #9

Closed IrmatDen closed 2 years ago

IrmatDen commented 2 years ago

Hi,

Thanks for this great addon, I discovered the late image fog recently and was worried no replacement existed... I discovered your module yesterday while browsing foundry's list as googling led me nowhere.

I made a small change to keep persisted areas as described in https://github.com/CarlosFdez/world-explorer/issues/6 as it's a feature I also desired. By default, it's disabled to respect your intention regarding pf2e's feat.

Let me know if any changes are required to this PR and thanks again :)

CarlosFdez commented 2 years ago

I'm sorry, I didn't notice this PR was up! I'll take a look at it.

CarlosFdez commented 2 years ago

Alright, so two things: 1) Try to maintain the same style as the rest of the codebase. For example, { on the same line as the if statement. Consistency is important. 2) Try to avoid updates to data occurring in the render. Perhaps try to do this in the update and create token events in index.js instead.

IrmatDen commented 2 years ago

Thank you for the feedback :)

  1. is simple to fix, I'll get to it this evening; I blindly followed my own conventions, sorry for that!
  2. Could you point me to some API wiki or code examples for this please? It's basically my first foray in foundry module code, so it's just a bit "arcane" to me right now. Still, I think I understand the logic of your feedback (not impact rendering performances, maybe even improve threading performances on foundry backend?)
CarlosFdez commented 2 years ago

It wouldn't improve threading performance. Its really only for standalone rendering performance and separation of concerns. Foundry uses Hooks to handle most types of events that occur, and short of subclassing and replacing core types we rely on Hooks to respond to things

https://foundryvtt.com/api/Hooks.html

Documents represent objects in the system, and they have a CRUD lifecycle of create update destroy. When these happen, events are fired. There's the pre and not-pre version for both.

If you look at the index file, you'll see some of these in practice. They're used to trigger rendering updates when tokens move.

PS: I've since realized btw that there's a way to convert from grid coordinate to real pixel coordinate, but I'm scared to make that update because we don't have a reliable way to migrate users who have already existing maps....

IrmatDen commented 2 years ago

Thanks a lot for taking the time to explain that; I'll get to it when possible. With christmas coming, I'm not sure I'll update the PR before the end of next week, but I'll get to it.

IrmatDen commented 2 years ago

Hi Carlos, Please let me know if you spot any other mistakes/possible improvements.

Happy holidays :)

CarlosFdez commented 2 years ago

Happy holidays to you as well. This looks significantly better, thanks for this.

I'll add a reset (with prompt) button to the controls when I get a bit of time and that should be enough for a release.