-
Notifications
You must be signed in to change notification settings - Fork 399
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
add Codec
s for FluidVariant
and ResourceAmount<FluidVariant>
#3419
base: 1.20.2
Are you sure you want to change the base?
Conversation
I thought IDEA had automatically added imports needed when I copy-pasted these lines from a mod of mine, but apparently it didn't |
Please add some unit tests for this, make sure that the change works as expected. |
…d so tests `FluidVariant.CODEC`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specific format is something that needs more discussion. 81000 is not the most user friendly, and it would be nice if users also had the option to use a different format.
@@ -27,4 +30,9 @@ | |||
*/ | |||
@ApiStatus.Experimental | |||
public record ResourceAmount<T> (T resource, long amount) { | |||
public static final Codec<ResourceAmount<FluidVariant>> FLUID_VARIANT_CODEC = RecordCodecBuilder.create(instance -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has no business being here, fluid-specific code should go in the fluid
subpackage of the transfer API. This could probably go in FluidVariant
.
@@ -43,6 +50,12 @@ | |||
@ApiStatus.Experimental | |||
@ApiStatus.NonExtendable | |||
public interface FluidVariant extends TransferVariant<Fluid> { | |||
public static final Codec<FluidVariant> CODEC = RecordCodecBuilder.create(instance -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably a MapCodec
in case some people want to use it inline with other fields?
public static final Codec<FluidVariant> CODEC = RecordCodecBuilder.create(instance -> | ||
instance.group( | ||
Registries.FLUID.getCodec().fieldOf("fluid").forGetter(FluidVariant::getFluid), | ||
NbtCompound.CODEC.optionalFieldOf("nbt").forGetter(variant -> Optional.ofNullable(variant.getNbt())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, the NBT parsing code might read small numbers as a different datatype than int
, that should really be checked.
Moved the resource amount codec. This was enough for my use case and I'm not interested in putting more work into it. I'm also not familiar with data fixers beyond the common uses, so you'll have to find someone else for any codec shenanigans If the 81000 denominator for fluids is not user-friendly, why was it used in the first place? Is there a non-leaking abstraction for it now? |
It is used internally, but should not necessarily be exposed to the user. I think being able to input as floating point buckets or millibuckets is more user-friendly, and should be offered as an option. Not sure about the exact format, here are a few options: {
"amount": {"unit": "bucket", "amount": 0.1 }, // a bit verbose
"amount": "0.1 B", // parsing is a bit tricky but probably reasonable
"amount": "100 mB", // could also support mB with the same format
"amount": "8100 d", // maybe also droplets
} Personally I like the string with a mandatory unit suffix. I can add the code to your PR if you want. |
If doing fancy codecs, why not have one of "amount", "millibuckets" or "buckets". Basically – with variant also "flattened" – like this: {
"fluid": "minecraft:water",
"amount": 40500
},
{
"fluid": "minecraft:water",
"millibuckets": 500
},
{
"fluid": "minecraft:water",
"buckets": 0.5
} If hiding that fluids use denominator of 81000, why not hide the fact that there are some "fluid variants", which users might never have heard of? |
Yeah we should definitely hide the "variant" too, IMO. I think |
If there are codecs for |
we can start with the fluid codecs only IMO, that makes things simpler |
Adds
Codec
fields for these two types, which might be useful for the new recipe serializers, which now useRecipeSerializer#codec
for reading from json, instead of receivingJsonObject
and reading it imperatively like it was before.