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

Use GetSprinklerTiles for Default Sprinkler Coverage for higher compatibility #5

Closed
wants to merge 1 commit into from

Conversation

rtrox
Copy link

@rtrox rtrox commented May 17, 2024

Currently, RangeHighlight leverages GetModifiedRangeForSprinkler to calculate the mask for sprinklers. SMAPI (and Stardew), also expose a GetSprinklerTiles method, though which already has the exact covered tiles as output -- this method is significantly easier for Mod Authors to override, which means leveraging this vanilla function will make it much easier to be compatible with more mods. Note that this is the method used by UIInfoSuite 2, as well.

Why?
My interest here is building compatibility with Line Sprinklers Redux (my mod). Given the purpose of the mod (spiritual successor to hootless' Line Sprinklers mod), I can't leverage GetModifiedRangeForSprinkler.

Testing
Image of my changes working correctly for both vanilla sprinklers and my modded sprinklers, with and without pressure nozzles:
TestGif

@jltaylor-us
Copy link
Owner

The problem with this (aside from it being silly to copy PointsToMask so that it's in two places instead of shared) is that it more than doubles the amount of allocation that happens for each sprinkler object when the highlight tiles are being computed, which could affect performance. My guess is that it wouldn't matter for the majority of people, but at the same time the majority of people don't have another mod installed that changes the sprinkler shapes and so won't see any benefit, either. It's still a pretty good idea, though, so I'll make a configuration option for it so that people trying to run SDV on a potato can revert to the existing implementation if they want to.

@jltaylor-us
Copy link
Owner

I've added this functionality as the default behavior in the 4.2.0 release (with a configuration option to revert to the old behavior).

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

Successfully merging this pull request may close these issues.

None yet

2 participants