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.

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.

3 participants