will issue #1 be reverted in Neoforge?
#1.20.5/6 Snapshots
clear bane
true jacinth
We don't currently fix either of those bugs - We did briefly before some pvp client weirdos came and complained it was cheating
magic sierra
oh
some pvp client weirdos?
They can't just modify the attribute in the client with a mod?
true jacinth
They could, the vanilla server validates the range as if the bug was fixed but the client doesn't permit it by default
but idk
magic sierra
Mojang could simply fix this bug...
foggy steeple
Maybe those pvp client people could stop cheatingβ¦ oops, I mean βusing quality of life toolsβ
digital drum
is it snapshot day?
clear bane
tomorrow
tacit tiger
Twitter
fallen igloo
pre1?
clear bane
he doesn't say that 
spiral radish
<:thinkies:932605814995292192> pre1?
pre-pre-1
summer furnace
it is a funny way to say "No snapshot today, we hope to release it tomorrow instead"
clear bane
yeah...
I don't think it will be a pre
restive raft
at what point do we just add a Tweet bot in this thread solely for slicedlime 
fallen igloo
idk if that is possible for free
restive raft
then the answer is simple: to the fediverse we go! https://mastodon.gamedev.place/@slicedlime
fallen igloo
well zapier has no direct triggers for either mastodon or twitter (both need another application) and sending a webhook is a premium feature (they have a direct message to channel action tho)
summer furnace
he's also on bluesky
no idea what kind of API bsky has
made by @fierce niche
fallen igloo
well I wanted to not use another bot
clear bane
make a server, that creates a webhook
summer furnace
meh, the more the merrier or whatever
elder swallow
next order of business: getting actions to run on the snapshot branch
the easiest (but not ideal) way would be to open a draft PR against 1.20.x
that would also setup PR publishing and therefore publishing of the snapshots hehe
prisma locust
@elder swallow I wonder - would it be better to use the last commit's time as the timestamp, rather than the current system time?
otherwise it seems I'll have to write some cursed logic in my CI to detect what version got published to maven local
digital drum
is this available to use in mods? this helps port mods easier as well
spiral radish
https://neoforged.net/news/neoforge-snapshots/
As of this post, these versions must be built manually. They are not available from our installer or our Maven repository.
elder swallow
<@298082405745426433> I wonder - would it be better to use the last commit's tim...
not necessarily a fan of that
spiral radish
If you know what your doing then yes, it's possible, but also as the blog post said, you shouldn't be releasing versions of mods for this
prisma locust
not necessarily a fan of that
do you have a better solution in mind?
elder swallow
as usual, what are you trying to do in the first place π
you could just publish a version to a snapshot maven of yours and reference that
prisma locust
as usual, what are you trying to do in the first place π
have my mod's CI compile the snapshot version, publish to maven local, and then compile the mod against it
I do not have a maven I can use to host Neo snapshot builds myself, so the CI needs to rebuild it each time
elder swallow
another potential solution is that we publish these builds to our snapshot maven
slow prism
smh pr publishing is a smart idea that wouldn't involve whole new infra
elder swallow
does the "pr maven" remain up forever?
slow prism
mhhm
elder swallow
(i.e. if I close the PR does it stay there)
slow prism
mhhm
elder swallow
indeed
magic sierra
Mojang seems to have started fixing regressions from previous snapshots, so probably the pre-release phase is coming soon.
clear bane
not really?
there is no indication to that
just that they moved the snapshot to tomorrow
magic sierra
By "soon" I mean in a few weeks.
summer furnace
tbh they are "late" already
can't be much longer until 1.20.5, if they still plan on releasing 1.21 in late june
clear bane
I mean...
maybe
just maybe
they have been working on 1.21 for a good while now
but. they have different people fixing up 1.20.5
meaning that the nbt changes and such, were done to prep us for 1.21
and then drop the nuke
magic sierra
Is the port/24w10a branch supposed to be buildable?
elder swallow
yes
magic sierra
RegistryOps<JsonElement> registryops = net.neoforged.neoforge.common.conditions.ConditionalOps.create(RegistryOps.create(JsonOps.INSTANCE, p_321612_), net.neoforged.neoforge.common.conditions.ICondition.IContext.TAGS_INVALID);
^
method RegistryOps.<T#1>create(DynamicOps<T#1>,Provider) is not applicable
(cannot infer type-variable(s) T#1,T#2
(argument mismatch; IContext cannot be converted to Provider))
method RegistryOps.<T#2>create(DynamicOps<T#2>,RegistryInfoLookup) is not applicable
(cannot infer type-variable(s) T#2,T#2
(argument mismatch; IContext cannot be converted to RegistryInfoLookup))
where T#1,T#2 are type-variables:
T#1 extends Object declared in method <T#1>create(DynamicOps<T#1>,Provider)
T#2 extends Object declared in method <T#2>create(DynamicOps<T#2>,RegistryInfoLookup)
/mnt/76561DD3561D94C9/NeoForge 24w10a/NeoForge/projects/neoforge/src/main/java/net/minecraft/world/level/biome/BiomeSpecialEffects.java:231: warning: [removal] BIOME_INFO_NOISE in Biome has been deprecated and marked for removal
double d0 = Biome.BIOME_INFO_NOISE.getValue(p_48097_ * 0.0225, p_48098_ * 0.0225, false);
why this?
blazing valve
Eclipse?
magic sierra
OH
spiral radish
oh I forgot to regen patches I think
smhmh
frail hatch
oh I forgot to regen patches I think
#project-talk message
dry stumpBOT
pupnewfster referenced
elder swallow
well easy fix in the end π
magic sierra
/neoforge debug_blockentity_renderbounds true is broken
also, is this normal?
[18:31:51] [Render thread/WARN] [minecraft/ClientPacketListener]: Entity Skeleton['Skeleton'/12843, l='ClientLevel', x=-60.50, y=-15.00, z=44.50] does not have attribute neoforge:swim_speed
[18:31:52] [Render thread/WARN] [minecraft/ClientPacketListener]: Entity Zombie['Zombie'/13111, l='ClientLevel', x=111.50, y=-1.00, z=37.30] does not have attribute neoforge:swim_speed
[18:31:52] [Render thread/WARN] [minecraft/ClientPacketListener]: Entity Bat['Bat'/12753, l='ClientLevel', x=10.75, y=32.69, z=96.25] does not have attribute neoforge:swim_speed
[18:31:52] [Render thread/WARN] [minecraft/ClientPacketListener]: Entity Bat['Bat'/12672, l='ClientLevel', x=19.72, y=-16.92, z=11.54] does not have attribute neoforge:swim_speed
[18:31:52] [Render thread/WARN] [minecraft/ClientPacketListener]: Entity Drowned['Drowned'/13218, l='ClientLevel', x=18.50, y=35.00, z=-42.50] does not have attribute neoforge:swim_speed
[18:31:58] [Server thread/INFO] [minecraft/IntegratedServer]: Saving and pausing game...
misty crow
The render bounds debug renderer is my fault. I misinterpreted how the model view matrix passed around in level rendering relates to the PoseStack used by consumers of the various events fired from level rendering. I'll push a fix for that later
plucky brook
π that's a thing? I'll need to use that
magic sierra
what is /spawn_armor_trims?
plucky brook
spawns armor stands with all armor trims
similar to how the debug world has all blockstates
magic sierra
it's neoforge only?
plucky brook
no, that vanilla, but dev only
magic sierra
oh
median wasp
It should be on all NeoForge dev envs, because NF enables the Vanilla dev flag in dev
frail hatch
wait there was a corresponding vanilla bug and scenario it could occur?
I didn't even realize that it was possible in vanilla when making that
reef parcel
vanilla's new gui stuff doesn't fix that yet?
magic sierra
nope
reef parcel
surprising
magic sierra
Another bug in the port: there seems to be weird desynchronization when the player places a block.
stiff galleon
can you be more specific
magic sierra
Well, sometimes when I place a block, the client behaves as if the block was never placed (no hand animation, no sound), but the server places the block.
like this
idk if it's a Vanilla bug, or NeoForge bug
median wasp
This happens when the client believes a block placement is invalid
The client always sends the click packet regardless of if it thinks the placement is valid
But if the placement is valid here, I'd say it's a Neo bug that the client thinks it's invalid
dry stumpBOT
Crash caused by the Ender Dragon
Created Gist at the request of @stiff galleon: https://gist.github.com/NeoCamelot/7d8bda7bf60ca559766a4d447e7ba273
stiff galleon
not this shit again ```
java.lang.NullPointerException: Cannot invoke "net.minecraft.world.entity.EntityDimensions.eyeHeight()" because the return value of "net.minecraft.world.entity.Entity.getDimensions(net.minecraft.world.entity.Pose)" is null
can we just remove the size events/stuff and direct people to using the attribute that mojang is adding?
my guess is it would fix that null pointer commoble linked
and presumably cover all use cases?
true jacinth
Yeah we should just axe it
summer furnace
I have always been a proponent of removing old features that don't quite make sense in the new version, and then adding something back if people still have a need of it
wild pine
any reason the background panorama speeds up when in the mods menu?
or is just an optical illusion and isnt actually speeding, just seeing less so it looks sped up
summer furnace
not optical illusion
I'm going to guess some patch duplicated a method call
and it's incrementing something twice
rigid walrus
The ScrollPanel class currently renders the default background texture only if a world isn't loaded but renders a custom gradient if a world is loaded
wild pine
is the port/24w10a branch supposed to work in userdev when published locally?
i mean i am able to setup a workspace with it and compile my mode against it
but when i try launch the client in intellij or gradlew runClient in cmd, it crashes saying some log4j method could not be found
java.lang.NoSuchMethodError: 'java.lang.Object org.apache.logging.log4j.util.LoaderUtil.newCheckedInstanceOfProperty(java.lang.String, java.lang.Class, java.util.function.Supplier)'
whats weird though, if i use WSL (ubuntu) and run the same command gradlew runClient, the game launches fine and loads my mod
elder swallow
It's supposed to work in userdev but I haven't tried it π
digital drum
I might try later today
elder swallow
should we throw in-dev if FluidStack.equals is called?
boreal flint
elder swallow
I am asking because FluidStack does not implement equals at all anymore
magic sierra
Is there a snapshot today?
magic sierra
Ok
summer furnace
should we throw in-dev if `FluidStack.equals` is called?
I see the point in throwing because people may expect .equals() to continue working, but I really really hate that this means anyone who understands .equals is a by-reference comparison is also prevented from using the class in a set unless they explicitly use an identity-based set instead of a "standard" one
It is probaby the correct thing to do regardless, I just don't like it :p
rigid walrus
I just realised that the background for the mod list screen is currently being rendered 3 times for 1 frame
clear bane
looks better now, imho
magic sierra
Nice
clear bane
and a repost in #squirrels-π¦ too
summer furnace
smh people it's not that hard.
if it talks about minecraft, #general
if it isnt' even about minecraft, #squirrels-π¦
only if it's specifically about 1.20.5 snapshots as related to the changes mojang made and how they will affect the porting effort, it should be here
elder swallow
I see the point in throwing because people may expect .equals() to continue work...
well let's go for it - happy to reconsider π
summer furnace
I just realised that the background for the mod list screen is currently being r...
that would explain why it moves too fast -- it's updating 3 times each tick :P
elder swallow
thoughts?
summer furnace
hm what's the reason for skipping validation in records?
elder swallow
they always override equals and hashcode
summer furnace
oh right
elder swallow
it's just to make sure that you don't use e.g. a fluid stack as a component
WHICH I DID like an idiot
summer furnace
oops
would there be a point of calling getMethod in line so that the || short-circuits the second getMethod call?
it should be slightly faster, but it's only in IDE so maybe it doesn't matter
rigid walrus
https://twitter.com/Minecraft/status/1767564702333247862
This tweet got posted in the game announcements channel category in the official Minecraft discord server
Twitter
wooden tendon
they always override equals and hashcode
There is no way to validate this otherwise?
elder swallow
not really, the registry is a Registry<DataComponentType<?>>
elder swallow
btw I just realized that DataComponentType is an interface not a class
wooden tendon
I would rather flip the check on its head
elder swallow
which means that we can make our own extended type if we want to
wooden tendon
if the equals and hashcode is not in the type of the component itself then it errors
elder swallow
we can't get the type class from a componenttype
wooden tendon
I don't really want to have think of the case where somebody has some abstract class
Why not?
summer furnace
could use getDeclaredMethod there instead of getMethod
elder swallow
I don't really want to have think of the case where somebody has some abstract c...
well it's correct as I wrote it
you can't instantiate an abstract class π
summer furnace
you can create subclasses though
and class X extends Y {} where Y implements equals & hashcode but X doesn't would still be problematic
elder swallow
getMethod walks the class hierarchy
wooden tendon
you can't instantiate an abstract class π
No but you can put the equals and hashcode in it, while the underlying type that is instantiated adds more stuff
summer furnace
which is what Orion is concerned about
wooden tendon
and `class X extends Y {}` where Y implements equals & hashcode but X doesn't wo...
Correct
elder swallow
`getMethod` walks the class hierarchy
see this
summer furnace
yes but you check if it's declared in Object
elder swallow
that's why I check for method.getDeclaringClass() == Objects.class
summer furnace
Orion is suggesting we check it's declared in the actual type
wooden tendon
method.getDeclaringClass() == clazz
summer furnace
and DISALLOW the case where X extends Y, and equals/hashcode is only declared in Y
wooden tendon
Is basically what I am asking you to implement
summer furnace
aka, getDeclaredMethod()
elder swallow
well idk, there could be valid cases where a subclass doesn't need to override equals
the main goal here is to educate modders about the system
wooden tendon
well idk, there could be valid cases where a subclass doesn't need to override e...
That same argument could be said for the Object case as well
elder swallow
if they go for more complex setups it's on them
wooden tendon
What if a component does not need an equals or hashcode at all
Then the declaring class is object
elder swallow
the only case where that would make sense would be a singleton component
wooden tendon
Which do exist
elder swallow
at which point you should probably ask yourself why not use a boolean π
wooden tendon
I have singleton capabilities
Which represent pieces of logic
Markers
And are used to identify internal pieces of logically processed objects
My point is
elder swallow
components are data though, not logic
wooden tendon
Either do it properly
And disallow them all
Or don't do the check
This really feels halfassed
elder swallow
no, it's just how I chose to do it π
both can work, but since we are adding checks I don't want to overdo it
rigid walrus
Vanilla uses Unit.INSTANCE for singletons for components
wooden tendon
components are data though, not logic
Components are data holders!, They themselves are not data. They represent it. They can represent the piece of information that something is processed
Which can be a singleton
Or any other construct
You don't know
And assuming in an API that something is not possible is how we get into problems with API design
I would suggest to not do this check
elder swallow
Vanilla uses `Unit.INSTANCE` for singletons for components
it does
wooden tendon
Or enforce it to be a record at all times (which is my preferred solution)
elder swallow
however that is an enum and should therefore implement equals π
I have to check
wooden tendon
Or enforce the checks so that it is always in the leave
elder swallow
yeah it has an equals implementation in Enum.class
wooden tendon
You are adding this check, to ensure that people properly implement the equals and hashcode, so that comparisons are easier and better.
elder swallow
Or enforce it to be a record at all times (which is my preferred solution)
that doesn't work, vanilla uses non-records too and there's good reasons not to use them
elder swallow
Or enforce the checks so that it is always in the leave
that's probably not going to work for enums (such as Unit.INSTANCE)
because Enum has a final equals method
wooden tendon
Can enums be used as components here?
elder swallow
yes
elder swallow
why?
wooden tendon
Because the API allows Object
wooden tendon
Yeah and I can pass new Object()
And your system will fail
Even though it is allowed to be used as a data component
elder swallow
and in doing so you will break copying and comparisons
which is the entire reason why there is a check in the first place
wooden tendon
Why?
elder swallow
because when you copy a stack, the components are shallow-copied
elder swallow
well it's way more performant π
wooden tendon
Yes but it makes 0 sense
elder swallow
that's why they also have to be immutable (which we sadly can't check)
wooden tendon
Because previously it was a deep copy, due to the nature of NBT
elder swallow
this is really nice, it makes stack copies a lot cheaper
wooden tendon
Yeah and a lot more dangerous as the same time
I understand your issue here...
That is a pickle for sure
elder swallow
this is also one of the reasons why we can't just blindly replace attachments by components in BEs, entities, etc
wooden tendon
I would have expected these things to have a codec attached
So that you can clone them efficiently
And properly
elder swallow
it's quite rare to be changing the data inside of a stack after all
wooden tendon
It is also a lot more dangerous
wooden tendon
it's quite rare to be changing the data inside of a stack after all
Not really
A lot of people mutate data in a stack
elder swallow
yes but there are orders of magnitude more item transfers where stacks are frequently copied
wooden tendon
For example tools, based on energy
Or or backpacks
Or magnets which are toggleable
elder swallow
all of that stuff is player-triggered and therefore only happens comparatively rarely
wooden tendon
yes but there are orders of magnitude more item transfers where stacks are frequ...
I would say that entirely depends on your context
restive raft
eh. if the data component API essentially requires (deep) immutability (even if we don't see that specific code statement), it is fine to rely on that immutability to avoid costs in copy operations
and it seems Mojang went that way
wooden tendon
eh. if the data component API essentially requires (deep) immutability (even if ...
Yeah but they don't
The API does not enforce it
wooden tendon
A good start would have been to make the components Records
That would at least ensure base type immutability
restive raft
it's why Guava's ImmutableCollections recommend using the explicit immutable collection type (i.e. ImmutableList in place of List) in method parameters or return types if immutability is part of the contract
wooden tendon
Yes
But the point still stands
The API clearly allows mutable objects
Just its sementical usage does not allow it
rigid walrus
elder swallow
here is an example of a non-record component
wooden tendon
here is an example of a non-record component
Yeah but that is a completly mutable object
elder swallow
it's not mutable hehe
restive raft
mutable in what sense?
wooden tendon
It could have been a record
Well it is not mutable probably
But that is purely an implementation detail
elder swallow
they wanted to guard mutations on the underlying map I suppose
restive raft
"immutable" is a separate concept from "being a record"
wooden tendon
Because p_331044_ can still be mutated outside of teh scope
Even after a stack is cloned
wooden tendon
"immutable" is a separate concept from "being a record"
records are by nature immutable at least on their field level
restive raft
before records existed, we considered classes that had no setters or any way to change the fields of the object to be immutable
wooden tendon
Yes to make something properly immutable, it needs immutable fields
wooden tendon
before records existed, we considered classes that had no setters or any way to ...
No actually. Their are several books out there on this topic. And they specifically do not define that as Immutable.
And object is only truely immutable if it and its contents are not modifiable by external access to the references
Which is a different then having a setter or a getter
But that is besides the point
I guess tech is right
There is no really great solution here
restive raft
I simplified my explanation -- I agree on that definition of immutability
wooden tendon
And some guarding is better then none
But this really needs some very very very good documentation
From our end
restive raft
but that does not refute the point that records are not the sole way to achieve immutability
wooden tendon
because this is shoot yourself and the entire community in the foor territory
wooden tendon
but that does not refute the point that records are not the sole way to achieve ...
You are right, but in modern J17+ they are a very easy and good start to indicate that you want immutability in your semantical usage of the object
restive raft
I assume the API on Mojang's end is documented adequately on its objects needing to be deeply immutable
and I assume Mojang have their reasons for not using records for data components
elder swallow
But this really needs some very very very good documentation
I'm planning to have it in the update blog post
maybe even in all caps
restive raft
perhaps so they don't have to have boilerplate to the effect of DataComponentType<ItemDamage> and record ItemDamage(int damage) in multiple places
wooden tendon
maybe even in all caps
Yes
elder swallow
DO NOT MUTATE A DATA COMPONENT EVER EVER DO NOT PLEASE ARGHHHH THEY'RE COMING FOR YOU
wooden tendon
If it were up to me, I would also log if a data component does not have an equals and hashcode in the concrete type in dev
Yes it is allowed and won't crash
restive raft
(as in, having to surround anything that isn't a record which they wish to use as a component, into a record, and avoiding auto-boxing and more boilerplate)
wooden tendon
but it is for me something really headscratching
elder swallow
well I suppose that a log isn't the end of the world
we can also exclude enums from the check
i.e.
-> no hashCode or equals at all -> crash
-> no hashCode or equals on the target class -> log
wooden tendon
(as in, having to surround anything that isn't a record which they wish to use a...
It not being a record, is then also an indication that maybe, maybe you did something wrong in your design.... But yeah wrong topc
wooden tendon
i.e.
-> no hashCode or equals at all -> crash
-> no hashCode or equals on the ta...
Yes that would be my prefferred solution
elder swallow
ok that is fine by me as well
wooden tendon
Where Enum is skipped just like Record
And we should write in the documentation that people should design their API around record
restive raft
It not being a record, is then also an indication that maybe, maybe you did some...
an indication, but not the sole one, and needs to be supported in context
wooden tendon
And only in exceptional circumstances use a none record type
Obviously as discussed it is not a clean solution
But it helps to get started
Maybe also point to things like Immutable Collections from guava
And the like
elder swallow
[11:49:54] [pool-7-thread-1/WARN] [ne.ne.ne.co.CommonHooks/]: Data component class class java.util.ImmutableCollections$ListN does not override equals and hashCode. Make sure that they are correct. Keep in mind that they must also be immutable. lol
wooden tendon
LOL
I don't know if there is a way to check for that
We should not log that for reasonable defaults
Where Record and Enum are probably a start
elder swallow
yeah that is the only log when initializing vanilla items atm
wooden tendon
True
But I mean ImmutableCollections are also a reasonable object
Like Enum and Record
So them being there makes a reasonable amount of sense in my eyes
But that this thing does not in and of it self implement equals and hashcode, has me raising an eyebrow
elder swallow
well it extends from ImmutableCollections.AbstractImmutableList
wooden tendon
Which probably implements it right?
wooden tendon
You might want to check if the object extends AbstractImmutableCollection and also accept those, like Record and Enum
Or at least AbstractImmutableList
I don't think anybody is going to be so crazy to extend that
And then tack on data
elder swallow
these classes are jdk internal
elder swallow
the entire ImmutableCollections class is package private
restive raft
mmmmm, API-impl isolation
elder swallow
Guava ImmutableLists have the same problem
but honestly: I think that we will catch most cases with the check against Object already
wooden tendon
Fine
rigid walrus
@elder swallow I don't have write access to the NeoForge repo so I had to pr it
rigid walrus
no since I mainly work on NeoForm and help with snapshots
elder swallow
beautiful
elder swallow
no since I mainly work on NeoForm and help with snapshots
we can either add you to the neoforge maintainers team or have you go through PRs
as you prefer
blazing valve
Wait, why does it print FluidStack as the problematic class?
elder swallow
because I did this...
blazing valve
Ah
elder swallow
which is wrong and exactly why we need that sort of check π
the question is now what to use for FluidStack components
personally I would love an immutable fluid stack class
π
summer furnace
I really don't like "Stack" for fluids :p
but π€·ββοΈ
do components need to be immutable, or even benefit from it?
I haven't looked at the code but I would assume most of them are mutable, thinking of like, Damage, Custom Name, etc.
elder swallow
the component object must be immutable
summer furnace
weird
elder swallow
if you need to change the damage you replace it with a new component object
summer furnace
so they are creating a whole new instance when the damage changes? that seems like a horrible design choice to me
elder swallow
what's the problem?
summer furnace
feels wrong
elder swallow
in exchange, stack copies become dirt cheap
this will massively improve the performance of IItemHandler et al
summer furnace
it probably isn't wrong, it just feels wrong
and yeah I suppose being able to copy the component references as-is is nice
is the component list itself immutable though?
or does copying a stack still make a copy of the list, just shallow copy
elder swallow
or does copying a stack still make a copy of the list, just shallow copy
it makes a lazy copy of the component map
how cursed would this be? 
it would be the perfect return type of IFluidHandler.getFluidInTank
instead of SERIOUSLY: DO NOT MODIFY THE RETURNED FLUIDSTACK
fallen igloo
how cursed would this be? <:thinkies:980924387333115914>
since this does not exist for itemstack, very
elder swallow
we could consider a similar thing for item stacks
rigid walrus
When building the initial components for items they also use an interner
elder swallow
yeah I patched in a check for the component class there
fallen igloo
we could consider a similar thing for item stacks
that would make us have to make it non final which will prompt people to do stupid things
and sealing classes is not supported by AT
wooden tendon
It makes the api so much better to have these immutable
elder swallow
I would prefer having ItemStack, ItemStack.Mutable and ItemStack.Immutable tbh
elder swallow
or BaseItemStack (read-only), ItemStack (mutable) and ImmutableItemStack
it's just that you'll get a runtime crash if you try to mutate it
whereas with a different hierarchy you could express in the API whether the stack can be mutated or not
I will also note that it's super cheap to convert between mutable and immutable stacks since the entire DataComponentPatch can be shared thanks to lazy copies
summer furnace
since this does not exist for itemstack, very
instead of rejecting the immutable fluidstack on this basis, we should do the opposite and have an ImmutableItemStack :P
rigid walrus
we can seal it _if we want to_
knows about a mod that makes ItemStack non-final
summer furnace
people should assume that itemstacks returned from stuff like getStackInSlot are immutable
and if anyone does mutate the stack, they will have to learn to fix that
elder swallow
wouldn't it be better to return a class or interface like ItemStackView directly
elder swallow
yeah of course
the main advantage of an immutable subclass is that you can pass it to any function that expects ItemStack
and vanilla has a ton of these
however vanilla also does stack mutation a lot and you might accidentally give an immutable stack and cause crashes down the line
I hope that whatever we do mojang will copy the idea and fix their pervasive stack mutation
clear bane
https://twitter.com/Minecraft/status/1767564702333247862
This tweet got posted i...
I do not like that I see bedrock buttons....
median wasp
Well presumably that is Bedrock Edition
median wasp
Yeah, so they can take screenshots on whatever
median wasp
They're just pushing bedrock
Nah, they tweet things from both editions (they do push BE, just not here)
summer furnace
BE has more total players afaik
younger players too
1 to 2 hours until snapshot
fallen igloo
BE has more total players afaik
only if you count all platforms, if you only count PC then java has more
mostly switch and xbox
clear bane
yes but bedrock means all platforms
fallen igloo
Registry refactor when
they are doing it incrementally already
clear bane
what about OpenGL upgrades, and a full render overhaul
and just give the finger to Apple?
fallen igloo
what about OpenGL upgrades, and a full render overhaul
won't happen because mac
clear bane
read the message under that
fallen igloo
read the message under that <:harold:932605815406342174>
they will never do that and you know it
clear bane
foggy steeple
When does Mojang add En passant to Mc?!
summer furnace
only if you count all platforms, if you only count PC then java has more
yup but for the purposes of the social media based PR, all is what counts, and that tilts the balance hard toward bedrock
stray bay
I haven't looked at the code but I would assume most of them are mutable, thinki...
isn't damage just an int?
median wasp
I haven't looked at the code but I would assume most of them are mutable, thinki...
They're all records
reef parcel
damage is just an int
median wasp
It's a record around an int, no?
median wasp
Oh it is, interesting
reef parcel
which i mean. is not mutable. you can't call int i = stack.get(DAMAGE); and i++ and expect it to update in the component
summer furnace
yeah thankfully
imagine if numbers were by-reference
1 = 2; and now everyone referencing 1 gets the value of 2
magic sierra
Note: https://www.minecraft.net/en-us/article/minecraft-snapshot-24w11a (error 404 rn)
spiral radish
That's assuming it is a snapshot (probably) and not a pre release
magic sierra
Yeap
vale holly
Note: https://www.minecraft.net/en-us/article/minecraft-snapshot-24w11a (error 4...
https://www.minecraft.net/en-us/article/minecraft-preview-1-20-80-22
This reveals some of the details, including both new items. ||We got a weapon finally||
digital drum
is it out now?
spiral radish
No, the version manifest hasn't been updated
digital drum
now?
fallen igloo
Twitter
elder swallow
that's the new weapon yeah
open kindle
spiral radish
elder swallow
the custom item predicate patch will need updating I guess ^^
maybe that fixes the blur idk
open kindle
Question, can we make the Snapshot Alarm role selectable in id:customize?
elder swallow
I think it grants access to #wisdoms-private atm - be careful
but I'd generally agree
open kindle
I think it grants access to <#1100546449713934426> atm - be careful
it does not, because I can't access that either
spiral radish
No, it should be a ping only role currently
summer furnace
This week we're expanding your arsenal with a smashing new weapon, the Mace! Use this weapon's special smash attack while leaping from a high place and watch your enemies get knocked back. The longer you fall, the harder you hit β but make sure you time it right! Your fall damage is only negated if you land the blow.
neat!
open kindle
what channel would that be?
cloud raven
elder swallow
wisdoms
cloud raven
Go nuts with the role
spiral radish
It did originally give access to another channel
open kindle
alr then I'll add it to the role selector
cloud raven
#maintainer-applications ??
spiral radish
It did originally give access to another channel
I gave it to someone thinking it was solely a pingable role, but that was fixed rather quickly
spiral radish
<#882405720929222737> ??
It was either wisdoms or internaltalk
frail hatch
Remove embedded! He is a maintainer now
cloud raven
i used "view server as role" and i can see maintainer applications as snapshot alarm
spiral radish
open kindle
Community Porter can be deleted as a whole
short cove
I wish they included pictures of the new stuff
cloud raven
huhhh i have a manual user override on that channel for some reason, so view server as role didn't work
spiral radish
<@&1067092163520909374>
<https://www.minecraft.net/en-us/article/minecraft-snaps...
Snowman available
short cove
Too much effort to go look at the new sherds and trims
open kindle
@cloud raven if you're ok with it, I'm gonna delete Community Porter
spiral radish
USE_DEVONLY π
restive raft
mmmm, BlockBox
lean anchor
Is that basically just BlockPos#betweenClosedStream(AABB) ?
open kindle
looks like it, yeah
should I make an announcement for the snapshot alarm role being a public thing now?
frail hatch
custom predicate patches are no more!
entity variant as well?
elder swallow
yep
spiral radish
should I make an announcement for the snapshot alarm role being a public thing n...
Likely, although may be better to ask in one of the -talk channels
reef parcel
how the heck do you use gitcraft. it keeps trying to get version metadata for a combat "snapshot" on some discord server
true jacinth
how cursed would this be? <:thinkies:980924387333115914>
The problem with this is that the immutable must be the parent class, not the child, otherwise you will get untraceable crashes from people handing the subclass around
true jacinth
If we make FluidStack immutable and add FluidStack.Mutable it can work out
but its a big change for mods
frail hatch
In mekanism I call the following methods the given number of times:
FluidStack#setAmount: 3
FluidStack#grow: 6
FluidStack#shrink: 1
Granted some of that is that I have helpers for my tanks that call those, so then I basically can easily change the implementation detail
elder swallow
I'd rather make a completely separate thing since I want the abstraction to also work for items
true jacinth
It may be weird trying to solve it via wrapper when we have the authority to solve it properly for fluids
elder swallow
Well I value symmetry between fluid and item stacks more
stray bay
what if the concrete class is mutable and the ['immutable'] fluidstack is an interface, same as how Component works
quiet condor
my role isn't special anymore :(
elder swallow
What the hell π
elder swallow
what if the concrete class is mutable and the ['immutable'] fluidstack is an int...
I have an idea but it will have to wait for tomorrow
Re the current snapshot, I don't see a reason to port to it. Best to focus on 24w10a for now
digital drum
interesting
fallen igloo
#builds message welp I'm gonna start on this (shouldn't be too bad, just a bunch of patch nuking)
stiff galleon
I missed the earlier conversation, what if I want to use a Block as a component
blocks don't override equals/hashcode
median wasp
Yeah, I disagree with the check. There are plenty of singletons that aren't enums
stiff galleon
don't make me mixin into norge to disable stupid validation
median wasp
While the check is good in theory, there are a lot of objects that have (and should have) identity equals and hashCode
lime galleon
I agree with Commoble that the validation shouldn't exist. There are ways to get around it that aren't mixins, although they're silly.
For your own types, you can always override equals to implement reference equality, even if it's pointless. But it bypasses the validation. This doesn't help for Block, though, in which case, you can use this very silly wrapper:
record RefEq<T>(T t) {}
You can now store a RefEq<Block> as a component. Hence bypassing the validation, just with some extra pointless indirection. Mildly annoying.
blazing valve
I missed the earlier conversation, what if I want to use a Block as a component
We whitelist known singletons
?
I mean, the validation likely whitelists primitive type wrappers too π
stiff galleon
what if I make my own singleton
blazing valve
If it's that important to you, the validation could come with an associated annotation to declare that you know what you are doing β’οΈ
stiff galleon
or we could just not put the validation in
reef parcel
I don't get why neo would add validation like that. At most, add a jdoc that tells users to be mindful about it
blazing valve
I strongly disagree
lime galleon
The only "known singletons" whitelisted are records and enums, if the commit is to go by.
blazing valve
People will fuck this up in the common case
Not catching those common mistakes with validation
is just inviting pain and suffering
stiff galleon
People will fuck this up in the *common* case
especially once they jarjar my lib that mixins out the validation
blazing valve
If you have a smart "i know what I am doing" case for the non-common case, you gotta go the extra mile and declare your own downfall π
blazing valve
especially once they jarjar my lib that mixins out the validation <:harold:93260...
And people will be able to detect which mods are likely candidates for causing issues based on including it. Every one wins.
Debugging and finding which mod fucked this up in a large modpack is a nightmare
wooden tendon
especially once they jarjar my lib that mixins out the validation <:harold:93260...
The check is disabled in production
So you really don't need to distribute shit
You just need to supply a proper equals and hashcode
So that everything works
stiff galleon
no, that's stupid
stiff galleon
why should a singleton override equals and hashcode
wooden tendon
It is enforced by the design mojang makes!
wooden tendon
why should a singleton override equals and hashcode
Even mojangs singleton does it
stiff galleon
lime galleon
Then, Orion, why would a data component of Block not work under mojang's design?
wooden tendon
Then, Orion, why would a data component of `Block` not work under mojang's desig...
Because it requires a proper implementation of equals and hashcode for the patching system in data components and the copying of itemstacks to work
So if you want a Block
Then you need to wrap it in something
That does the equals and hashcode, based on for example the registry name of the block
blazing valve
Blocks should work fine since they are essentially singletons π€
wooden tendon
Blocks should work fine since they are essentially singletons π€
Without the required equals and hashcode sadly
So they don't
blazing valve
using identity equals/hashCode is fine for them
stiff galleon
That does the equals and hashcode, based on for example the registry name of the...
well, no, I'd just override -- yeah that ^
wooden tendon
Well yeah
blazing valve
That is domain knowledge, however
stiff galleon
identity hashcode and super.equals
wooden tendon
And in reality people can fuck this up so badly so quickly
That enforcing this (in dev only)
Is just a good idea
Even though I fully understands commobles position
As I said before I wanted it to be Records only....
So that it helps to indicate in the API that it should be immutable and comparable
But yeah
No dice
Because mojang decided it wanted things like tooltip logic, and Unit.INSTANCE
blazing valve
I can see the position of allowing known singletons explicitly, or declaring "yes I DECLARE this is guaranteed to be immutable"
But not "we should not have a validator"
lime galleon
So if you want a Block
Then you need to wrap it in something
Then is there any issue with attaching this, for example, as a component.
record Box(Block b) {}
Something like a marker interface right
wooden tendon
Then is there any issue with attaching this, for example, as a component.
```jav...
That will work π
Because of the domain logic of how Blocks work
Basically they are instance specific (as in kind of singletons on their own right)
lime galleon
It is enforced by the design mojang makes!
So, the only reason Block doesn't work by itself, is because NeoForge has a validation that prevents it. Not because it's mojang's design. Because it is otherwise a singleton.
wooden tendon
So, the _only reason_ `Block` doesn't work by itself, is because NeoForge has a ...
Yes and no
Yes it would work without the check
But no it is kind of a bad idea, because it implicitly depends on the specific domain behaviour of Blocks/Items etc
The point of that check
Is that you make that domain behaviour explicitly known to the system
So that the semantical behaviour of that system is guaranteed
blazing valve
I mean our validator is in the domain. We can whitelist at the very least the known Minecraft singletons (Item, Block, BlockState(?)), I suppose
wooden tendon
I mean our validator is in the domain. We *can* whitelist at the very least the ...
We could probably whitelist all registry types
prisma locust
is there a valid use case for wanting a block as a component though
wooden tendon
And their subtypes
blazing valve
Probably?
wooden tendon
is there a valid use case for wanting a block as a component though
Probably
I mean BlockItem?
prisma locust
fair enough
wooden tendon
I see that really as a component for example in the future
stiff galleon
There's a non-zero chance that mojang just makes one blockitem with a blockstate component in the future
prisma locust
please no
blazing valve
unlikely since the item itself is a singleton too
wooden tendon
Unlikely yeah
median wasp
I mean our validator is in the domain. We *can* whitelist at the very least the ...
StateHolder > BlockState for this case
median wasp
We could probably whitelist all registry types
Yeah, those are explicitly compared with identity in many places
reef parcel
It'd be more a BlockItemType and the block items themselves defined in data
wooden tendon
The fact of the matter is: we can iterate on this
median wasp
But yeah, I'd say just whitelist all registry types and any other singletons (like StateHolder)
wooden tendon
The current special cases do not need to be set in stone
We can start with what we have now
And if the community wants more types
It is just a couple of lines of code
And then it is releassed 2 days later
Done
kind sable
Sooo did we get the obvious awesome new feature this time?
stiff galleon
no, banana dog is still a myth
slow prism
We could probably whitelist all registry types
that's not exactly easy
true jacinth
We could just whitelist Holder and tell people to use that
stiff galleon
We could just whitelist Holder and tell people to use that
fair
fallen igloo
hmm now that we have the porting progress on main I can use the action to port to the next snapshot
misty crow
We could just whitelist Holder and tell people to use that
Holder.Direct would like to have a word with you π
stiff galleon
well, that's already a record and therefore whitelisted
misty crow
How did I miss that... 
fallen igloo
stiff galleon
No signature of method: java.lang.String.formatted() is applicable for argument types: (String, String, String) values: [20.5, 24w10a, 20240314.204139]
Possible solutions: format(java.lang.String, [Ljava.lang.Object;), format(java.util.Locale, java.lang.String, [Ljava.lang.Object;)
median wasp
How did I miss that... <:facepalm:932605815540572180>
I don't think it does in 1.20.4
misty crow
It's also a record in 1.20.4. I was looking at 24w10a sources at that moment anyway though
slow prism
<@561254664750891009> FIX IT
you can fix it too, it's easy
slow prism
well then what do you think you ought to do
fallen igloo
maty you broke it you fix it (you can even use the web editor)
median wasp
It's also a record in 1.20.4. I was looking at 24w10a sources at that moment any...
I didn't remember that Direct was a record. I also don't think I've ever used Direct
(except indirectly with playsound)
slow prism
maty you broke it you fix it (you can even use the web editor)
no I can't use the web editor on mobile so unless you feel like waiting 14 hours to get it fixed you're going to do it yourself
Β―_(γ)_/Β―
fallen igloo
no I can't use the web editor on mobile so unless you feel like waiting 14 hours...
what did you even break?
slow prism
nothing lol
more like what did tech break https://github.com/neoforged/NeoForge/blob/port%2F24w10a/build.gradle#L18
to make the buildscript incompatible with j8
did he actually use string.formatted in a language with interpolation....
damn it tech smh
and the mix of gstring and string... ew
fallen igloo
@elder swallow ^
wooden tendon
J8 Compat is not relevant
Due to the use of spotless NF Development we need J17 as far as I understand it
fallen igloo
well that workflow doesn't have J17 and it worked when starting this snapshot cycle
slow prism
Due to the use of spotless NF Development we need J17 as far as I understand it
it needs 11
formatted is a java 15 thing
but also groovy has string interpolation lol
fallen igloo
I don't think it uses J8 (probably J11)
wooden tendon
Hmm
true jacinth
if it needs 11 just bump it to 17 then
wooden tendon
That is kind of my point
elder swallow
Block components are useful
However we also need some form of checking
Potentially we can add a hook for modders to say "this class is a singleton and therefore acceptable"
E.g. CommonHooks.markComponentTypeAsValid(clazz)
fallen igloo
if it needs 11 just bump it to 17 then
the workflow currently does not define a java version but uses the one groovy comes with
fallen igloo
12 reject files
rigid walrus
https://github.com/actions/runner-images/blob/releases/ubuntu22/20240310/images/ubuntu/Ubuntu2204-Readme.md
Java 11 is the default java version installed on github-hosted runners
fallen igloo
we have another case of the patcher putting the class header after a constant
plucky brook
rigid walrus
Noticed LocalPlayer localplayer = this.minecraft.player; on the on the last line of an if statement
Seems like it wasn't a decompiler error
fallen igloo
do we still need PotionColorCalculationEvent? or can I nuke the patch in LivingEntity (it now works with a List<ParticleOptions>)
ok, only client and networking left
3 rejects left
misty crow
We could just whitelist Holder and tell people to use that
I just went over all of vanilla's data component types and we do in fact need to whitelist Holder because it's used for instruments (the goat horn), otherwise you can't even get into the creative inventory. I added Holder.class.isAssignableFrom(clazz) locally to fix that
true jacinth
what if someone makes a nonsense Holder implementation to skirt restrictions 
stiff galleon
Potentially we can add a hook for modders to say "this class is a singleton and ...
@KeyFor?
why is the checkerframework in my project libs
blazing valve
`@KeyFor`? <:thinkies:932605814995292192>
I thought about annotations but that would limit it to your own types. Maybe both, though? π€
prisma locust
what if someone makes a nonsense Holder implementation to skirt restrictions <:t...
such a developer could also implement a contractually incorrect equals and hashCode, so it's not a concern
stray bay
why are we restricting what types people are allowed to use as components if Mojang doesn't
reef parcel
β¨ validation β¨
So people who don't read jdocs or those who are ignorant to proper implementations are forced to at least try not to screw it up
slow prism
we have another case of the patcher putting the class header after a constant
not again
stray bay
eh, i don't like it
true jacinth
Mojang probably doesn't need to
errant lion
Mojang doesnt make stuff so that modders will use it, its just a happy accident that we exist, or atleast that is prob what Mojang thinks.
elder swallow
what if someone makes a nonsense Holder implementation to skirt restrictions <:t...
Does DeferredHolder even implement equals and hashCode properly?
rigid walrus
For applying damage from the mace from falling it checks if the player is holding the mace in Player.attack
elder swallow
ToolAction time?
summer furnace
Mojang doesnt make stuff so that modders will use it, its just a happy accident ...
nah most of mojang these days are former modders, they just can't justify new features/designs based on modding, so they have to find excuses to do so while giving things a vanilla purpose
elder swallow
Well, if something benefits modding it often benefits minecraft itself as well
Since we generally want stuff to be more flexible;)
errant lion
Yep
true jacinth
Does DeferredHolder even implement equals and hashCode properly?
I'm not sure, I don't think I was around to review the retargetted PR to 20.2
It should just defer the equals/hashcode to the underlying resourcekey
elder swallow
That's probably gonna break symmetry of equals if Holder doesn't implement it correctly itself
So e.g. a stack with a normal Holder might not stack with a stack with a corresponding DH
stray bay
It should just defer the equals/hashcode to the underlying resourcekey
should direct holders try to find the object in the registry, then?
rigid walrus
I have a removal of ICustomItemPredicate locally along with moving the current neoforge item predicates to mojang's item sub predicate system
stray bay
or maybe direct holders are just always unequal to reference holders
true jacinth
should direct holders try to find the object in the registry, then?
Probably. I'm pretty sure the only real use of direct holders is during worldgen data load
rigid walrus
Also this is the registry value of mojang's item sub predicate registry: public static record Type<T extends ItemSubPredicate>(Codec<T> codec)
stray bay
we can probably add default impl on holder - kind() to determine reference vs direct, and i assume deferred holders are considered reference holders
true jacinth
Also this is the registry value of mojang's item sub predicate registry: `public...
Weird. Wonder why its not just a straight codec registry
stray bay
how many holder subtypes are there
just direct, vanilla reference, and deferred?
not too bad i guess
elder swallow
I have a removal of `ICustomItemPredicate` locally along with moving the current...
sounds good
should I squash the 24w11a port and push it to the main repo?
fallen igloo
yea remove sources and squash
elder swallow
ok
rigid walrus
I just found out that they implemented the same system for entity predicates
elder swallow
yep
coeh are you taking care of the item and entity predicate removal?
ok yeah Holders don't implement equals and/or hashcode
and DH.equals(normal holder) will return false
stray bay
is there anything that would break if we add it? [anything storing them in maps that assume identity equals]
rigid walrus
coeh are you taking care of the item and entity predicate removal?
yes
fallen igloo
coeh are you taking care of the item and entity predicate removal?
the patch for item predicate is gone already
elder swallow
yeah but we need to also remove the patch for entity predicates, remove our custom related classes, update our tool action predicate, and re-run datagen to update the loot tables
elder swallow
is there anything that would break if we add it? [anything storing them in maps ...
I suppose that we don't have the choice anyway
I can add a test for it too
stray bay
i mean we could simply not
elder swallow
yeah that means that stacks that contain a deferred holder will not stack with stacks that contain a normal holder
that is unacceptable
stray bay
....oh, are components compared by equality?
elder swallow
yes
stray bay
i thought this was just about restricting allowed types
elder swallow
that is the entire reason why we check for equals and hash code
we're not in the business of adding arbitrary restrictions
but in this case it is far from arbitrary
stray bay
is there a way to, like, have some mechanism to lookup a comparison provider? that'd also let us fall back on serialization
elder swallow
I don't see why we'd do that
it's a step backwards compared to mojang's current design
stray bay
hmm... vanilla reference holders are interned anyway right? so there shouldn't be any impact to implementing it on them and on DH [do we want/need to touch direct holders?]
elder swallow
oh god, are there non-synchronized data component types in vanilla
we potentially need to check that else the creative inventory will nuke them
(I get capability flashbacks already)
true jacinth
there's what now
I would just go ping boq or smth lmao
That will cause vanilla bugs
elder swallow
summer furnace
mojang really needs to fix their creative mode lol
elder swallow
well no
they are potentially planning on adding components to non-itemstacks too
OK nevermind
it defaults to the regular codec if no stream codec is provided
summer furnace
that's good considering how the creative menu works
but it's really annoying we can't have server-only data in the stack
so much unnecessary synchronization
true jacinth
The entire creative menu is fucked ye
server only data would be great if it wasn't so weird
pretty sure its notchcode still
elder swallow
well it's great for mods that stacks are the same on the client side
e.g. "drag filter from JEI" can't work well if the client doesn't see the full stack
true jacinth
Usually the only data you would hide would be like backpack contents or other bulky data
stray bay
i'm not sure it's really worth supporting
backpacks that support 27 slots or less aren't going to be any bulkier than shulker boxes, those that support more should arguably implement their own off-stack system
true jacinth
its less of an issue now, especially with the packet splitter in place
rigid walrus
Turns out when specifying a sub predicate for entities the predicate becomes
"predicate": {
"type_specific": {
"type": "neoforge:piglin_neutral_armor"
}
}
for items its
"item": {
"predicates": {
"neoforge:piglin_currency": {}
}
}
that is odd
elder swallow
adding this
elder swallow
I just went over all of vanilla's data component types and we do in fact need to...
did you add that somewhere? cause I can enter the creative inventory just fine locally
stray bay
is there a reason not to allow adding classes that are otherwise valid?
elder swallow
that's what this method is for
stray bay
i am asking why the throws
elder swallow
cause that probably means that someone doesn't know what they're doing
stray bay
it's annoying to have to change if refactoring a type that previously wasn't a record into one
idk, i guess it's not a big deal, i just don't see the harm either way
wooden tendon
is there a reason not to allow adding classes that are otherwise valid?
Yeah because you brick the system otherwise.
The need for equals, hashcode and immutability is semantically defined in the component system. There is no syntactical requirement for it.
However to prevent you from shooting yourself in the food and everybody else in the room in the head when you process an itemstack, we enforce this requirement to prevent this mistake
stray bay
right but i assume this enforcement is done by doing, like, "if(set.contains(clazz) || expensive_reflection_checks(clazz))"
what is wrong with adding classes that would pass the checks to the set
elder swallow
why would you call the method in the first place if your classes already pass the check?
there is no reason to call markComponentClassAsValid if the class is already valid
stray bay
refactoring, i'm mainly curious about what Orion is saying and I think he misunderstood my question
elder swallow
if you refactor it will be trivial for you to fix the test
in fact, being able to remove the check would be a good reason to refactor to a record
wooden tendon
refactoring, i'm mainly curious about what Orion is saying and I think he misund...
No i did not....
Given class A, already existing, you asked me for a reason as to why we could not allow adding that class A even if it is otherwise valid.
And I told you why
Because the component system has ssemantical requirements that are not expressed in the syntax of the api
stray bay
Given class A, already existing, you asked me for a reason as to why we could no...
my question was why not allow adding records to the whitelist, not why not allow adding random [non-record, non-whitelisted] classes to the components
i think you're still misunderstanding what I was talking about 'adding' things to
elder swallow
sounds like a pointless discussion in any case
fallen igloo
we should disallow collection types as well
fallen igloo
because an array list has equals and hashcode but is mutable
foggy steeple
Yeah because you brick the system otherwise.
The need for equals, hashcode and ...
Whatβs the actual tangible result if someone messes up
elder swallow
we'd have to special case them sadly
elder swallow
Whatβs the actual tangible result if someone messes up
if you fuck up equals: stacks that should stack will not stack and vice versa
if you fuck up immutability: modify multiple copies of the stack at the same time
summer furnace
we can't protect all the cases
summer furnace
if people use arraylist so be it
fallen igloo
if you fuck up immutability: modify multiple copies of the stack at the same tim...
hmm quantum entangled stacks
restive raft
a cursed thought would be a special mode that tests for collection immutability by trying to remove from the collection, expecting and catching the resulting exception
summer furnace
even if we went that far, which IMO is way too far
all it takes is someone making their own custom class that holds a List in a field
record Lulz(List<X>){}
and I think it's 100x more likely people will use mutable fields in their class, than a List<>
we just need a big warning in the documentation
rigid walrus
if you fuck up immutability: modify multiple copies of the stack at the same tim...
Or you modify every stack that has that item and for equals you could also turn a component into another one of the same type (like what mojang did with attributes)
elder swallow
fallen igloo
<:thonk:932605815221788672>
and how is that useful?
frail hatch
also fluidstack doesn't include amount in equals and hashcode unless you changed that
wooden tendon
<:thonk:932605815221788672>
I would say a FluidStack.Immutable is more apropo
elder swallow
it's incredibly useful for transfer but that is just spitballing / early POC at the moment
elder swallow
also fluidstack doesn't include amount in equals and hashcode unless you changed...
now it doesn't implement it at all
summer furnace
i don't see the empty fluid as being a measure of amount
frail hatch
now it doesn't implement it at all
that is definitely going to lead to bugs
summer furnace
but rather of content type
frail hatch
of people who use fluidstacks as keys
summer furnace
empty/air vs something else
elder swallow
I might override it just to make it throw
using a fluid stack as a key is pretty stupid anyway π
elder swallow
you could use a fluid variant though π
frail hatch
nothing mutable should
just don't mutate it
elder swallow
I would say a FluidStack.Immutable is more apropo
I don't think we should add an immutable subclass. Someone will inevitably end up doing player.setItemInHand(new ItemStack.Immutable(xxx)) and cause a crash down the line
I am thinking of a separate ImmutableFluidStack/ImmutableItemStack class (but ideally with a better name)
wooden tendon
I would then not name it FluidStack
But something like ImmutableXXXContainer
So that it is immediatly clear what the fuck we are talking about
And maybe even make the Stack variant extend it
So that if somebody is just interested in a Container
They can accept the common type
I don't think that having two completely distinct types is a great idea
Alone from the required brainpower to apprehend the concept
summer furnace
ideally the immutable variant would be the superclass
wooden tendon
Yes
elder swallow
in an ideal world we would have the mutable stack, an immutable stack, and potentially also a count-less "stack" for use in maps, transfer, etc
fallen igloo
But something like ImmutableXXXContainer
container has the implication of inventory tho
elder swallow
- some common superclass / superinterface
wooden tendon
container has the implication of inventory tho
The name is not really the point, I would not use variant though
elder swallow
but that is too many classes anyway
wooden tendon
but that is too many classes anyway
Well the countless one should be read-only anyway
So it could be the root type in the tree
elder swallow
yeah definitely
wooden tendon
Then the immutable one on top
fallen igloo
yea variant and container are not good for the usecase
wooden tendon
And then the mutable one on that
Maybe with some generics you can get away with only a handfull classes
That would be worth it in my opinion
Because this problem with mutability has plagued us since -beta times
elder swallow
generic over item/fluid stack? that would be a good step towards unification
wooden tendon
generic over item/fluid stack? that would be a good step towards unification
Yeah
elder swallow
or well, I mean "common abstraction" is more appropriate than "unification"
wooden tendon
If your whole concept is that the FluidStack and ItemStack types should have the same API surface
elder swallow
it is
wooden tendon
Then I would argue that their Immutable, and Countless super types should simply be generic
summer furnace
record ItemVariant(Item, Components) {}
interface ItemHolder { ItemVariant getVariant(), Item getItem(), Components getComponents(), getCount() }
record ItemResource(ItemVariant, count) implements ItemHodler
class ItemStack implements ItemHolder
Or something like that
Point is
There are many ways to skin this cat
And, IMHO, we should not stop with this road, just because it adds 5 or 6 classes
summer furnace
yeah ItemVariant and ItemHolder could be made into like, Variant<Item> and ResourceHolder<Item>
wooden tendon
We have systems which add like 20 classes
And we give 0 fucks about that either
Given that this system would solve something we have been strugling with since the beginning of modding, I would even add 100 Classes to solve it
frail hatch
There are many ways to skin this cat
don't be mean to the kitties πΏ
wooden tendon
Sorry
summer furnace
// Generic
record Variant<T>(T, Components) {}
interface ResourceHolder<T> { Variant<T> getVariant(), T getResource(), Components getComponents(), int getCount() }
interface ResourceStack<T> extends ResourceHolder<T> { void setCount(int); <X> void setComponent(ComponentType<X>, X); }
// Specialized
interface ItemHolder extends ResourceHolder<Item> { Item getItem() -> getResource(); /* needed for ItemStack existing method name compatibility */ }
record ItemResource(Variant<Item>, int count) implements ItemHodler
class ItemStack implements ItemHolder, ResourceStack<Item>
interface FluidHolder extends ResourceHolder<Fluid> { Fluid getFluid() -> getResource(); /* consistency */ }
record FluidResource(Variant<Fluid>, int count) implements FluidHodler
class FluidStack implements FluidHolder, ResourceStack<Fluid>
I'm not sure we'd gain much from those being generic though, and we can't just add an abstract ResourceStack<T> superclass to ItemStack
wooden tendon
```java
// Generic
record Variant<T>(T, Components) {}
interface ResourceHolder<...
Sure we can add ResourceStack<Item> to ItemStack
summer furnace
as an interface yes, but we can't pull the logic up
wooden tendon
as an interface yes, but we can't pull the **logic** up
Hmm, maybe we should make them an interface then
Not sure
summer furnace
what does Stack add over ResourceHolder?
setCount and setComponent?
slow prism
```java
// Generic
record Variant<T>(T, Components) {}
interface ResourceHolder<...
thats a lot of objects
wooden tendon
what does Stack add over ResourceHolder?
setCount and setComponent?
Probably yeah
summer furnace
I added those to the mockup
wooden tendon
Again it should be doable
elder swallow
It will need a lot of iteration to get an acceptable design for sure
The end game for me (not for 1.20.5 but maybe we get to do it between 1.21 and 1.22) would be to use this hierarchy to simplify our transfer handlers as well
summer furnace
yeah
elder swallow
To fix all the "do not modify the returned stack", "this stack is yours", "this stack is not yours " crap
summer furnace
Variant<Energy>
FORGE_ENERGY = ENERGY.register("fe", ...) 
EnergyStack 
elder swallow
To be even clearer, I would like the interface for I/O to be java // Returns how much was inserted int insert(int slot, CountlessStack what, int howMuch, Action action);
elder swallow
`Variant<Energy>` <:thonkies:1130937086254534888>
`FORGE_ENERGY = ENERGY.regist...
Energy would probably keep its own cap
It's not worth dealing with slots etc for energy IMO
summer furnace
interface ResourceHandler<T> {
int insert(int slot, Variant<T>, int count, ...);
int extract(int slot, Variant<T>, int count, ...);
}
oops yes I forgot the slot too
elder swallow
Hehe yeah that'd be ideal
summer furnace
(that made no sense, reverted)
interface ResourceContainer<T> {
Variant<T> getContentsType(int slot);
int getContentsCount(int slot);
}
interface ResourceInventory<T> extends ResourceContainer<T> {
ResourceHolder<T> pickupContents(int slot)
ResourceHolder<T> swapContents(int slot, ResourceHolder<T>)
ResourceHolder<T> splitContents(int slot, int quantity);
}
π€
elder swallow
Well I'm not a fan of splitting the transfer interfaces into multiple pieces
It makes sense to be able to query the content from a resource handler
summer furnace
yeah I was picturing it as a superinterface
elder swallow
And in principle pickup/swap/split can be done with helper functions on top of insert/extract
summer furnace
I really don't think that's how pickup/place/split should work
it's the part I dislike the most about the current IItemHandler
pickup/place should have their own separate logic, not insert/extract
elder swallow
Isn't pickup just extract?
summer furnace
no
not to me, at least
in almost every single inventory I have implemented, pick up bypasses the restrictions of insert/extract
elder swallow
The usual solution is to have multiple handlers
summer furnace
yes, and I would like not to require that crutch in the API
that's why all my proposals for an item handler API refactor include this separate "gui handler" vs "automation handler"
summer furnace
in almost every single inventory I have implemented, pick up bypasses the restri...
of course the big exception for me right now is Ender-rift, where the whole inventory network is based on a concept of "transaction fee", by having an energy cost on insert/extract proportional to the number of slots present in the network
in that concept I do have to route everything through insert/extract just because using IItemHandlerModifiable would bypass too much :P
I would love immutable resources in that API because it would remove the possibility of someone mutating a stack without the energy fee taking effect :P
but separating automation and gui into different interfaces would still allow me to apply the appropriate fees
elder swallow
Hmmm
In my experience it's quite easy to manage an "internal inventory" and external wrappers
plucky brook
hmm quantum entangled stacks
new mod?
elder swallow
so what we really need is:
- FluidStack
- immutable FluidStack
- amount-less immutable FluidStack
ideally sharing some code but without overdoing it
lean anchor
FluidKey?
elder swallow
that would be the amount-less immutable one
lean anchor
Honestly the name "Stack" sucks, you don't stack fluids
FluidDefinition, FluidInstance, naming like that...
Or, you know. FluidType
elder swallow
the exact names are TBD (it's not the tricky part)
lean anchor
It's splitting the concept of what a fluid is vs how much there is
blazing valve
Im partial to Key, but only because we settled on that in AE
elder swallow
It's splitting the concept of what a fluid is vs how much there is
precisely
lean anchor
I mean you already have ItemKey and BlockKey
blazing valve
We do?
elder swallow
however I still want to keep FluidStack around very similarly to how we have ItemStack, since an essential goal is to have maximal symmetry between items and fluids
lean anchor
Well, ResourceKey<Item>