Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose aura logic for other grid type support #16543

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FolkvangrForgent
Copy link
Contributor

Description

This MR serves to expose the aura related classes and make a minor change. This will allow modules such as pf2e-hex to add support for additional grid types to the systems aura automation.

Changes

  • Move grid type check from checkAuras function to containsToken. This was done to make it so that modules extending the behavior only need to override containsToken.
  • Expose AuraRenderers, AuraRenderer, TokenAura, and EffectAreaSquare classes via CONFIG.PF2E to allow modules to access them to wrap functions.

@CarlosFdez
Copy link
Collaborator

This almost looks fine to me, though I wonder if those classes are exposed if we should not construct instances from the config instead of importing directly in our own uses of it, to discourage monkey patching and encourage subclassing. But this is something that @stwlam is more familiar with.

@FolkvangrForgent
Copy link
Contributor Author

Are other things in CONFIG setup to use subclassing? I can certainly look into changing it to a subclassing friendly implementation if that is the design the project is aiming for. I also just want to note the AuraRenderers, AuraRenderer, and TokenAura are the only things I am monkey patching. EffectAreaSquare is just exposed so I can create instances to use within my monkey patching.

@FolkvangrForgent
Copy link
Contributor Author

Hi Just wondering if you've had a chance to take a look at this MR @stwlam . I've been using a build with this in my home games and haven't run into any issues with the current implementation.

@CarlosFdez
Copy link
Collaborator

CarlosFdez commented Nov 3, 2024

So far I don''t think stwlam is amenable to exposing any aura logic at all, so I don't expect this will ever be merged.

You specifically just need some way to determine "is in aura"? Its kinda bad code design wise, but the best thing I can think of as a compromise is moving containsToken() to a check in token for isInAura(), as tokens are foundry api and thus overridable.

@FolkvangrForgent
Copy link
Contributor Author

Hi CarlosFdez,
If the system does not want to expose the aura objects then the token object having a function would be better than nothing. It would prevent me from piggybacking off the existing rendering calls but I could always try to implement my own render. I am curious what the hesitation for exposing the aura logic would be; I would certainly not be expecting a stable api or anything of that sort from it's exposure. I'd also love to add hex (and maybe even gridless) support directly to the aura logic in the pf2e system if this is something the system would be amenable to. Although currently I would want fvtt to expose some functions so I don't have duplicate versions of fvtt functions: foundryvtt/foundryvtt#11795 .

@shemetz
Copy link
Contributor

shemetz commented Jan 4, 2025

@stwlam Could this be reconsidered? The latest position on auras in gridless/hexes was "paizo didn't make firm rules, so the pf2e system won't try to support them, but a module could override this" -- and in cases like this, modules require the system to add some API support without increasing the maintenance load for the system devs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants