search

architectury / architectury-api

An intermediary api aimed at easing development of multiplatform mods.
https://discord.architectury.dev/
GNU Lesser General Public License v3.0
315 stars 56 forks source link

Make ArchitecturyFlowingFluid use the same Forge FluidType when the s… #467

Closed digitalseraphim closed 8 months ago

digitalseraphim commented 9 months ago

…ame ArchitecturyFluidAttributes object is passed in.

This fixes https://github.com/architectury/architectury-api/issues/447. This is the smallest change I could come up with. The only other way I could see to fix it would be to create a Forge specific version of the SimpleArchitecturyFluidAttributes class that contained a toForgeType or getForgeType, that cached the value itself, but you'd have to cast it, because the interface type is used for the AFF constructor, and we don't want Forge specific types in the interface, unless we also make a forge specific version of the interface, and that just seems to be getting out of control.. I think this is a nice clean solution to the issue.

CLAassistant commented 9 months ago

CLA assistant check
All committers have signed the CLA.

shedaniel commented 8 months ago

Does toForgeProperties also need to return the same value per the same attribute? If not, this PR can probably be simplified as follows:

image
digitalseraphim commented 8 months ago

Dealing with a power outage, doing this on my phone, so please pardon any shortcuts/typos.

There's a little cleanup here as well, as AFF doesn't need to override getFluidType, as the Forge implementation returns what it gets from the properties object returned by toForgeProperties. So there's also no need for the forgeType member. Instead of creating multiple FluidType instances that are just going to be thrown away, (as well as Properties objects), doesn't this make more sense?

On Tue, Jan 9, 2024, 8:53 PM shedaniel @.***> wrote:

Does toForgeProperties also need to return the same value per the same attribute? If not, this PR can probably be simplified as follows: image.png (view on web) https://github.com/architectury/architectury-api/assets/34910653/fa2d6816-ec5e-4649-a003-2186c2b25db5

— Reply to this email directly, view it on GitHub https://github.com/architectury/architectury-api/pull/467#issuecomment-1884066995, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB4DK6YDNIV7TD2NVKJKFLYNXYCHAVCNFSM6AAAAABBHUKOE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBUGA3DMOJZGU . You are receiving this because you authored the thread.Message ID: @.***>

shedaniel commented 8 months ago

That does make sense, thank you. I will clean this a little bit up (just small nitpicks) and this should be good to go!