Capability rework

[quaint yoke]

Exactly:

NeoForge already handled common cases such as chunk load/unloads and block entity creation/removal

Though @raven coral the one with block placement/removal seems... iffy

[rain tartan]

question: what happens in the case that a modder doesn't invalidate properly (when NeoForge doesn't do it)

as in, the worst case

[quaint yoke]

Any other mods keeping a cached reference to the capability will still have the old one

That's the worst case scenario

[brazen verge]

Except super rare situations like when a block is removed, sure

[quaint yoke]

That's only blocks without BEs, yes

If your capability has an associated BE, there's no common cases where you have to do invalidation

If it doesn't have a BE, then... actually I'm not quite sure why it works that way, I assume auto-invalidating stuff with that system would be too expensive?

But that's a question for Tech

And the answer for why it's being kept: mods are going to cache capabilities no matter what. It's inefficient for some not to. The invalidation system makes it less likely for mods to keep bad caches to capabilities around, by providing mods with a way to explicitly say "something changed!". Will it be perfect? Well, no, no system will be. Is it better than the alternative, which is mods caching capabilities and then keeping around references to dead caps when they ought to be updated? Yes

Will the average modder using caps actually care? No

The average capability is, I'm almost certain, a simple item handler on a block entity

In which case, there's literally nothing that needs done for invalidation

[brazen verge]

Is it better than the alternative, which is mods caching capabilities and then keeping around references to dead caps when they ought to be updated? Yes

that's my question, is the point

is it better?

are we incurring a performance penalty from having a semi-automatically handled invalidation scheme for something that might only ever be useful to a niche selection of mods

when they themselves could easily just have a native invalidation system

[quaint yoke]

That's the point, they... kinda can't. Because suddenly some mod does something that would need manual invalidation, and it literally cannot notify those mods about it and now you get undefined behavior. I'm talking mods having systems to cache caps which may be provided by blocks from other mods

[brazen verge]

put field in cap w/ method getter?

[quaint yoke]

And the performance penalty is minimal... with the exception of the case tech notably didn't implement automatic invalidation for

[brazen verge]

or just make a base interface for modders to handle

[quaint yoke]

Huh? How does that help?

[brazen verge]

rather than incurring any kind of potential penalty on neoforge's side

it doesn't, that's why I suggested interface lol

[quaint yoke]

That's... basically what this is? Let me try explaining the issue differently

[brazen verge]

yes

but without neoforge doing anything

is my point

like I'm speculating, I don't actually know how much cost this has on NeoForge's side

but if it's more than "basically zero", then it's question-raising

[quaint yoke]

Then... literally every mod that adds a capability to a block would have to do it themselves...

[brazen verge]

only mods that need to invalidate those caps

[quaint yoke]

No... that's not the point of invalidating caps...

The point is to invalidate them for other mods

[fleet gull]

I was planning on keeping the good ol Map<Block, API> for block apis anyway

[quaint yoke]

Here, let me give an example

[brazen verge]

I'm aware of what invalidating is for

[quaint yoke]

Mod A looks up ItemHandlers a lot. It can't do that every tick, so it needs to keep a cache of blockpos to capability. How does it know when to update this cache?

[fleet gull]

I yeet my cache when a blockstate changes

[quaint yoke]

The only way it can do that efficiently is if the blocks providing the capability somehow notify it when they update their capabilities or get places/broken

[fleet gull]

via NeighborNotifyEvent

[brazen verge]

yes but this is my point lol

[quaint yoke]

Keep in mind that this isn't just neighbors

[brazen verge]

it relies on you, as a modder, doing the right thing

which we all know doesn't happen terribly reliably

[fleet gull]

I'm not just neighbors

[brazen verge]

which then brings into question the validity of a global system, rather than an opt-in one

[fleet gull]

I have a big ol' map of blockpos to itemhandler somewhere

[quaint yoke]

Capability lookup can't always rely on neighbor updates because it might not be happening from a block

That's the point here

[fleet gull]

when a blockstate changes, which I detect via neighbor notify event, then I yeet the cache

err

I don't cache the itemhandlers

[brazen verge]

if there's no performance/cost of the system, then sure
But it feels like this is going to have an inherent time cost on neoforge's side, handling cap invalidation for the 99% of cases where it's useless

[fleet gull]

just the blockposs

I look up the itemhandlers when I need them

[quaint yoke]

Right, that what this is for

It caches the ItemHandlers

Cause that lookup can be expensive

[fleet gull]

I don't need to cache the itemhandlers though, I can just grab the blockentity from the level and ask the blockentity for them

[quaint yoke]

No, you can't

Block entities don't have caps

Block positions do

[fleet gull]

ew

[quaint yoke]

How expensive do you think cap invalidation actually is? It should be a pretty cheap operation if nothing has a cache up. As cheap as lookup I'd suspect. Beyond that, it's only fired for events which generally shouldn't be happening often anyways, like BE creations/destruction and chunk loading stuff

[quaint yoke]

Well, otherwise you can't get caps on composters.

[fleet gull]

compostors don't need caps though

[quaint yoke]

They do though

[fleet gull]

nah

[brazen verge]

you tell me, I'm raising the question

[quaint yoke]

They're item handlers

[fleet gull]

just stick a hopper between the composter and the item tubes

[brazen verge]

if the question of performance RE: invalidation hasn't already been raised then that's concerning in and of itself tbh, given cap validation's history

I choose to assume it's already been thoroughly thought about

[quaint yoke]

(that was a rhetorical question; they shouldn't be that expensive given the impl, though I've not benchmarked it because that's a pain and it's not my ballpark)

[fleet gull]

can we keep the old cap system

[quaint yoke]

That leaves you with competing standards

[quaint yoke]

That... should very obviously be a non-solution

[fleet gull]

nah, it works with my item tubes

[quaint yoke]

Yes but the point is that that's equivalent to ye olde "convert between these two mods" blocks that having a shared item capability is meant to prevent

And it's unintuitive to players, who have no idea about BEs vs normal blocks

[fleet gull]

wut

no, I mean instead of having the new cap system

[quaint yoke]

Given that the old one sucks... I mean I guess I don't have a say but seems like a bad idea

[quaint yoke]

Worth keeping in mind that the way invalidation works is a bit different than it was before; basically, there's a cache that forge has for block capability lookups. If a mod doesn't choose to use the cache when grabbing a capability, there shouldn't actually be anything to invalidate - it's not like before where LazyOptional was a thing

[fleet gull]

it sounds like the only thing the new one does is give composters itemhandlers, which we've just established is trivially solvable anyway by sticking a hopper in front of them

[quaint yoke]

But tech knows more than I do there because I've only glanced through the impl

[fleet gull]

at the cost of making everyone rewrite their itemhandling logic

[quaint yoke]

The hopper thing is not a solution... how am I, as a player, supposed to guess that even though this block holds items just like all these other ones, only vanilla hoppers and nothing from mods works on it?

That's a cop out from designing a system that doesn't have that issue

And sticking with the old system also forces mods to use BEs when they only really need a normal old block

Plus, this doesn't make you "rewrite your ItemHandlers"

The item handler interface is to my knowledge unchanged

The only thing that changed is how you actually provide the capability

[fleet gull]

I mean the parts of my code that interact with itemhandlers

[quaint yoke]

Sure, same could be said of the registry changes

[fleet gull]

have to change everything to get it from the block/level instead of the blockentity

[quaint yoke]

It's a necessary change because the current system is unfeasible in the long run

[fleet gull]

nah, caps have been feasible for years

[brazen verge]

I mean yeah I get the same vibe from the registry changes tbh lol
It feels like the change had more potential that was just never realised

[quaint yoke]

It hasn't been. The composter thing has been a consistent point of pain for players and modders alike since the composter existed

I'd know because I had a ton of pain getting stuff to play nice with composters back in 1.18

[fleet gull]

did you try putting a hopper on top of it

[quaint yoke]

... as a modder

[fleet gull]

as a modder if players ask me how to get stuff to work with composters I tell them to use a hopper

[quaint yoke]

If your solution is "put a hopper on it" might as well just remove all inter-mod or mod-and-vanilla item capability interactions and have it all use hoppers...

[fleet gull]

nah I'm not talking about intermod stuff, just composters

the inter-mod itemhandling stuff all works fine

[quaint yoke]

Unless mods use a block without a BE to store items...

[fleet gull]

that sounds hard

[quaint yoke]

And mod-and-vanilla interactions are as important or more important than inter mod interactions

[quaint yoke]

composters
It's fucking trivial

[fleet gull]

composters don't store items

[quaint yoke]

Sorry, accepts items and can have them extracted, whatever

Mods can do that too

[rain tartan]

to me, that's not a solution, that's a workaround

[fleet gull]

that's fine, to me it's a solution

[quaint yoke]

Workarounds aren't solutions. You're just brushing the problem under the rug

[fleet gull]

not if it's a solution, not a workaround

to me it's not a workaround, it's a solution

[quaint yoke]

"Oh, this mod works fine but can't extract or input items from chests without hoppers"

That would be an issue

[rain tartan]

well, not a solution in the sense that it solves the underlying problem, that I can't hook up mod-thing-A to a composter and have it work (within the reasonable confines of that mod-thing-A's design)

[quaint yoke]

Composters are no different

[fleet gull]

good thing I'm talking about composters, not chests

[brazen verge]

they're saying it's the same situation

[quaint yoke]

And your opinion on this is not universal. Many modders would consider those two to be equivalent situations; the ability to interact with composters is just as important as interacting with chests, and is all an important aspect of fitting in well with the existing vanilla infrastructure

[fleet gull]

neither is yours

[quaint yoke]

If your solution to someone not being able to do something they have no reasonable reason not to do is "don't do it"... you're just wrong

[fleet gull]

no, my solution is to put a hopper on top of it

[rain tartan]

suppose a user is given some manner of pipes and told "you can use this to insert stuff into blocks"
so the natural thought is "whatever accepts items, I can use these pipes for"
they would eventually try it on composters (like if they're mass-producing bonemeal) since they know composters accept items, find out it didn't work, and think it is a bug

[fleet gull]

if they report it as a bug I can inform them they need to use a hopper and they become informed

[quaint yoke]

...to clarify, a modder says "I want first class integration with all vanilla and modded item providing or accepting things, including composters". You're saying that the correct response is "no, you can't do that, you have to go through a hopper instead ". There is no good reason a mod shouldn't be able to do that

[brazen verge]

I mean that's kinda the definition of a workaround lol

[rain tartan]

you've not solved the underlying problem we're talking about here when we say "we cannot programmatically insert stuff into composters and other similar blocks without hardcoding special cases"
you've just solved "a user can't put stuff into composters"
which is a solution for that user, but a workaround for the original problem

[brazen verge]

when you're working around a problem, rather than solving it

[rain tartan]

(and even in the user's context, it seems pretty workaround-y to hand-wave it away as "composters are special -- use hoppers")

[brazen verge]

composter integration is actually really annoying as a player too tbh
It's a really tedious and limited block to use by hand, and you almost kinda need automation to make it any kind of viable

being inherently limited on that front is pretty significant

[quaint yoke]

Like, I understand the reticence with the capability invalidation stuff. Tech can probably explain the reasoning and details on that better than anyone else given that he's the one who designed and wrote all of it. But this bit? The composter thing was a huge sticking point of the old API. Trying to deny that is... really not a great approach. The point of an API like neoforge is to make something that promotes interoperability between mods and covers reasonable use cases - and that means having a sensible way of handling composters

[brazen verge]

I don't have an issue with the idea of the invalidation

I'm more questioning its usefulness on a global scale vs the cost of implementation at runtime

if the cost is basically zero, then I'm totally fine with it

if it has any kind of cost, then I'm questioning it

[fleet gull]

yeah I wasn't caching itemhandlers to begin with so any invalidation changes don't affect my tube graphs

[rain tartan]

I believe someone (I forget who exactly) tried to propose a block capabilities API via a PR in MCForge, but it languished due to insufficient manpower (to review, to test, to etc.)

[brazen verge]

I obviously haven't done any benchmarking on it, but I really want to assume someone has at the very least thought about it before implementing it

so I'm asking about that

[quaint yoke]

Like I said, I can only say a bit here and there about it - tech's the one who could tell you how big that cost actually is with more detail than "probably not much different than lookup, which is happening already, and not really much of anything most of the time unless the system using it is actively in use"

[rain tartan]

https://github.com/MinecraftForge/MinecraftForge/issues/8216 I had the suspicion who wrote it in the back of my mind

[brazen verge]

that seems pretty substantial to me

[quaint yoke]

But as I understand the invalidation system, the only places forge fires stuff itself are:

• BE creation/deletion
• chunk loading/unloading

[brazen verge]

the argument I get thrown about invalidation in the first place is that 'lookup is too expensive to do a bunch'

so if it's about the same cost as that.. that seems.. notable?

[quaint yoke]

Yes, that's the point. The invalidation only invalidates stuff that was already looked up

If the big old map of cached item handlers is empty... it has nothing to do

That's a cheap check

[fleet gull]

was anybody ever querying itemhandlers from blockentities stored inside shulker boxes or whatever

err not shulker boxes

those store items

but like, a pokeball for blockentities or something

[rain tartan]

you've lost me

[fleet gull]

probably not then

[quaint yoke]

I mean that's sorta the thing, you can query ItemHandlers from anywhere. There's folks querying them from entities I'm sure

[fleet gull]

I mean, querying itemhandlers from blockentities not-in-a-level, basically

maybe that's not a thing, blockentities probably get annoyed if they don't have a level+pos

[quaint yoke]

Yeah they don't like it

Most mods that do that sort of thing have them in a fake level anyways

(for the exact reason of "playing nice with mods that assume BEs are in a level")

[fleet gull]

what about BlockEntityWithoutLevelRenderer

[quaint yoke]

If you're shoving a capability on that you've got bigger issues

[fleet gull]

do mods actually use that for rendering blockentities without levels?

or do we just borrow it for rendering itemstacks

[quaint yoke]

Basically that yeah

[raven coral]

The invalidation system has basically zero cost if you don't use it, yeah

[serene minnow]

Itemstacks have a different cap then BE. So you move the data between the 2 with the blockentitytag. So you never have a BE without level?

[quaint yoke]

Good to know I understood it mostly right!

[raven coral]

OK I pushed but it seems wrong (?)

have fun I'll be back in 2 hours 😄

[stoic warren]

looks very wrong

you have conflicts somehow

[twin wedge]

spacing changed for 1.20.3 right?

so could be related to that

oh

it is just because the PR now also includes the 1.20.3 commit

[stoic warren]

skrem

tech

[twin wedge]

wait I am dumb

[stoic warren]

i now need to fix that so we can actually release 20.3

[twin wedge]

I missed the fact the 1.20.3 commit got pushed (didn't realized that happened yet)

[stoic warren]

how is it conflicting

lol

[twin wedge]

I think I was right

[stoic warren]

the diff shows everything

skrem

[twin wedge]

it has to do with the spacing

[stoic warren]

@raven coral i will

even pulling has conflicts

@mint current do you want to volunteer to fix tech's branch

my local repo is acting up

[mint current]

I can try

[stoic warren]

ok i have an idea

ok it's fixed

[mint current]

👌

[stoic warren]

anyone want to do a last approval on the PR?

auto merged enabled

expect the PR to be merged in around 8 minutes

let's see who i can voluntell

@mint current write an announcement for the initial 1.20.3 release w/ the caps changes linking to the blog post

[mint current]

I'm quite literally the worst person you could ask to write an announcement

[raven coral]

So what happened? Did GitHub not pick up the changes from the squash?

[twin wedge]

think technically your commit of 1.20.3 port and the actual pushed ones were "different" commits as far as it was concerned

and it kind of had a stroke

[stoic warren]

I fixed it using good ol' git reset

[raven coral]

lol the force-push says that there is no difference

[quaint yoke]

@raven coral the blog post has a few things you might want to change as they could be a bit misleading. In the examples form making capabilities with createSided and createVoid, it's really not made clear that if you want interoperability with other mods using IitenHandler (the most common use case) you'll need to use the pre made one not your own RL. Maybe mention that the built in forge caps can be found in Capabulities before that point in the article

But this is super exciting! Thanks for putting so much time into this change

[radiant minnow]

getData + setData 🤔
updateData?

[raven coral]

thanks, I clarified the blog post a little 🙂

[raven coral]

maybe but that's also kinda bloat ¯_(ツ)_/¯

[fleet gull]

something about the block-based capabilities's been bothering me

let's say that I have a system where I have a Map<Block, Behavior>, which I populate from json files in datapacks -- one json per block, I can set up a second loader if I need a second map, and the jsons are type-dispatched so mods can register custom serializers if needed

(I do this so that I can make vanilla gold/copper blocks conduct electricity )

is there any reason to use the new block capabilities instead of this system (for custom "capability" types, not ones that already exist like itemhandlers)

[quaint yoke]

Well, if you want other mods to be able to provide their own stuff in their own ways, or if you want BE based blocks to have their own systems for determining whether or not they expose stuff or what they expose, or the like

For some applications though, yeah, your system is what you'd want to use

[fleet gull]

mods can provide their own stuff in their own ways, by registering a custom serialize and writing a json for their block

the APIs on the Behavior all have worldpos context so they can be BE-based

[quaint yoke]

Right but that's different. Capabilities can be added to things you don't own

And can be implemented on things you do

[fleet gull]

you can add a behavior to a block you don't own by writing a json for the block you don't own, like vanilla gold blocks

[quaint yoke]

So, like, if I have a sometimes_conductor, a cap would let it only sometimes expose a capability or something

[quaint yoke]

Additionally, the cap providers are per block but the caps are per block location - if your cap involves reading or writing data, that's going to be far more efficient

Because you don't have to look up the BE every operation

[fleet gull]

yeah in my specific impl I cache the behavior per block location

[quaint yoke]

But if you're just attaching a number to blocks... probably no reason to use caps. If you expect mods to have to grab a BE to look up that number or whatever, caps would be better

[quaint yoke]

Right, and now you need to implement invalidation like caps already have and you've reinvented caps

[fleet gull]

thing is, I implemented all this before the cap rewrite

I didn't reinvent caps, caps reinvented me

[quaint yoke]

Well, you're probably not doing the same sort of invalidation

I assume you only need to do this at neighboring locations or whatever

But yeah, similar enough systems

[fleet gull]

I assemble a block graph -- each pos is connected to whatever positions it says it's connected to, so, yes, but the definition of "neighbor" is fluid

[quaint yoke]

Yeah, though how do you know to yeet the cache for a location?

[fleet gull]

blockstate update or manual invalidation

[quaint yoke]

Right, so just what caps do

[fleet gull]

okay, so we both invented the same system because the idea in general is good

[quaint yoke]

The advantage of caps is that you don't need your own custom system any more

[quaint yoke]

Exactly, yeah

If you still want the data stuff you can do that with caps too

[fleet gull]

but now it would be more work to unwrite everything I wrote than to keep using it

[quaint yoke]

Just attaching a lowest priority data based capability provider to everyty, I'd bet

[fleet gull]

plus I like the datapack aspect

[raven coral]

I mean, if you have something that works, don't bother changing it

(IMO)

[quaint yoke]

I mean fair. If there's no reason to use caps, you don't need to. The one advantage of caps is that it would avoid hard deps - people could register, say, a Supplier<Integer> or whatever generic Java class you want, if you make that the cap type, without a hard dep on your mod in dev - but really not a big deal. Use what works until it doesn't

[raven coral]

capabilities are nice because they do more than Map<Block, thing>, for example you can trivially add capability support to AE2 parts without AE2 itself knowing anything about the cap except that it exists

[fleet gull]

mine avoids hard deps too, if the builtin serializers in my power api are sufficient then they don't need any java

i.e. you could make an electric generator that provides x voltage when blockstate=on and 0 voltage when blockstate=off, with just the builtin serializers and no classes/interfaces from the api

[radiant minnow]

Avoids a double lookup

[raven coral]

But adds a lambda allocation heh

[radiant minnow]

If you tell me that allocating a lambda is more expensive than an additional HashMap lookup and validation of all parameters...

[raven coral]

IHM lookups are very cheap

[cedar oar]

allocating a lambda should definitely be more expensive than one hashmap lookup on a small sized map. if the lambda is "constant" though, the allocation only happens once so it would be faster afterward, but if it's capturing stuff and it allocates every time then I would think the object instantiation would be slower?

[quaint yoke]

Given that it's an identity hash map, and those lookups are dirt cheap? Definitely

[radiant minnow]

Well, whatever

At worst, can just AT the map and use it directly

[quaint yoke]

...I mean, why would you need to?

[cedar granite]

been too long since I've done this... if I have an intellij migration XML file (forge->neoforge in this case), what do I need to do to make intellij pick it up?

istr there's a specific intellij folder it needs to go in?

[noble rampart]

Yes, but did you try the gradle script that Tech posted a while back?

Oh, and this isn't really the right thread for that 😅

[cedar granite]

oops yeah wrong channel, thought I was in general 😳

(reposted there, but Tech did recommend using the migration xml rather than the script)

[brazen verge]

so I'm running into an issue with this implementation

namely - attached data objects that differ on client & server sides
because the attacher doesn't give a reference object, there doesn't seem to be a way to do any kind of context-sensitive object creation
was this thought about in implementation?

[cedar oar]

I have run into the lack of owner at construction time, but for other purposes

my solution has usually been to have a setOwner(whatever) called explcitly, eg in the player logged in event

it's not ideal but you could in principle use such a process to optionally attach client data to the object

[brazen verge]

I mean

if we're needing workarounds for a new system before it even leaves beta

it might be indicative of the system having a shortfall

rather than a workaround being the correct solution

[cedar oar]

yes, I asked already if it would be an option to have the owner passed through into the constructor

[brazen verge]

I don't see why it shouldn't

[cedar oar]

Tech said it was possible but it could only ever be a generic Object

[brazen verge]

no

[cedar oar]

since attachments are intentionally not object-specific

[brazen verge]

you could pass it right through from IAttachmentHolder

[cedar oar]

but the API wouldn't know

[brazen verge]

the API would know it's an IAttachmentHolder

tbh

I don't even really care if we have to blindcast

that's still a far superior solution than 'just do hacky workarounds'

[cedar oar]

also yes it could be an IAttachmentHolder (I didn't notice that interface :P) but that really doesn't add much vs Object

so pretend I said IAttachmentHolder, the part where it's generic and requires casting is still true

[brazen verge]

fine with me

[fleet gull]

all of my 1.20.1 chunk capabilities have a reference to the chunk

is that no longer possible?

[unique fiber]

it is, if the chunk is part of the attachment itself

but the attachment itself is no longer party to what object it's attached to

[fleet gull]

I'm only ever attaching this to chunks

[unique fiber]

it can contain anything, and it can also be attached to anything

should you want it to

[fleet gull]

no I don't want it to be attached to anything except chunks

[unique fiber]

then don't set it to attach to anything except chunks :p

that's what giga meant

[brazen verge]

Sure

Except you still don’t have any knowledge

Even sidedness appears to be impossible

Which feels pretty significant to me

[twin wedge]

Yeah I was working on porting a mod earlier today that uses entity caps in a broader way than what I have done previously and was wishing there was the context of what it is attached to as well. But I haven’t looked yet if there is a better way for me to restructure the code so that it isn’t necessary

[brazen verge]

Well atm I’ve got a pretty significant issue trying to deal with this reduced functionality over the previous system
Basically having to make a pseudo-provider wrapper around the damn thing so I can make it work

[quaint yoke]

Wait data attachments don't get passed the thing they're attached to when they're created?

... the fuck. Welp, there goes my nice easy 1.20.4 port for Tempest I guess

[brazen verge]

You have to provide a supplier of your data object

Not a function, or a getter

A supplier

Which is defined at registration

Its proving to be quite a problem

[raven coral]

It's relatively easy to add this back but you indeed only get Object or the holder type

[brazen verge]

perfectly fine

not having it at all is far more problematic

I'm gonna have to start doing some pretty hacky stuff to make this work without it

and mine's not even a particularly unusual use case

[raven coral]

Not sure about IAttachmentHolder vs Object. Might as well be a bit more specific I suppose

I need to think about how to pass this to the default instance supplier and the deserializer

[raven coral]

Can you link me some of your capability implementations where you capture a reference to the holder?

[radiant minnow]

I assume the pattern for this would then be instanceof-ing to verify the class type, right?

[raven coral]

Yeah

[brazen verge]

yeah

[brazen verge]

like on the old system?

[raven coral]

Yeah

[brazen verge]

https://github.com/Tslat/Advent-Of-Ascension/blob/0be2106bebb3851c5ba2cc9974e57936a867244d/source/content/capability/adventplayer/AdventPlayerCapabilityProvider.java#L23

[radiant minnow]

https://github.com/Tslat/Advent-Of-Ascension/blob/0be2106bebb3851c5ba2cc9974e57936a867244d/source/content/capability/adventplayer/AdventPlayerCapability.java#L15

[brazen verge]

?

[radiant minnow]

That constructor is actually the critical part

As you're attaching the server player data manager thing

And that one needs the player

[brazen verge]

shrug

I mean without completely rewriting it I don't see how you can do anything more than just provide the attachmentholder to the provider

[radiant minnow]

Also, I didn't want to navigate on mobile

[brazen verge]

converting it from a Supplier<T> to a Function<IAttachmentHolder, T>

which seems more or less fine to me

[raven coral]

That's the idea yes

Can be done in a backwards-compatible way too I think

[radiant minnow]

Does it work WRT source?

Because sometimes Java just doesn't know if you want a function or supplier, I've had it happen

[raven coral]

Should be reasonably obvious if we go from 0 to 1 parameter

But it's worth verifying

[radiant minnow]

Yes, but you see... Java isn't always reasonable lol

[stoic warren]

nah it will work

[raven coral]

The reason it was left out was because you can work around it. But given the amount of code written against the old capability system it makes sense to restore the functionality instead of asking everyone to rework their attachments fundamentally

[stoic warren]

we can keep the supplier versions, don't even need to deprecate 'em as not everyone will need it

[raven coral]

The serializer will be a bit trickier. Probably going to have two defaulted methods bouncing to each other

[brazen verge]

I disagree

it's not a small workaround

it's a fairly significant problem imo

[raven coral]

I agree, it's already hard enough to port as is

[brazen verge]

I agree with this though

[radiant minnow]

Also, just worth noting that if the intended solution was setters, that also leads to design problems as now the attachment might or might not be ready when the data is queried

[brazen verge]

^ among other issues

I'm not even talking about porting

if it's a rewrite it's a rewrite

but this functionality shortfall doesn't just require some basic workaround

there's actually a fairly significant implementation issue in requiring users to sort this out on their side

like for me I'd basically have to attach a provider to the player as a data object

then that provider lazily instantiates the actual data, which then self-sets the player

which is absolutely disgusting

[raven coral]

ah yeah cause you need more than just the player instance

but actually need to swap out the object entirely

[brazen verge]

correct

[raven coral]

I'll have a look soon™️

Not right now but hopefully tonight

[cedar oar]

it is possible just not during construction yet. at the moment you have to use chunk load event to call your own setOwner

[raven coral]

I just realized that deserialization with the attachment is always going to be on the server side for entities

[raven coral]

Chunk load? I would have made a helper method that gets the attachment and sets the owner heh 😛

[radiant minnow]

I would have just proxied the attachment

[raw hull]

All these would be replaced with attachments:

• PlayerData, which takes the Player, could be refactored, but does mean it would have an ugly "attached but not initialized" state
• Forging and Egg - both of which take the ItemStack, but, only because they serialize to stack NBT due to creative inventory/sync issues, which is fixed by attachments, so these are easy removals.
• Heat and Food, which do some form of "query a recipe based on the item stack, then attach (optionally) based on that", which becomes quite tricky to do without, and again would leave a really ugly "attached but not initialized/legal" state, and the default initializer is just invalid.

Ultimately lacking this feature means I'd probably have a lot of wrappers that look like

static A getDataForReal(T thing) {
    A attach = thing.getData(MY_ATTACHMENT);
    attach.setThingIfNotAlready(thing);
    return attach;
}

One concern that isn't just solved by "helper method that hides ugly state" is this, from the blog post

a default value supplier to create the instance when the data is first accessed, or to compare stacks that have the data and stacks that don’t have it;

In my case for both Heat and Food, the default value supplier is now invalid for some stacks, which means this would lead to some weird bugs.

[quaint yoke]

https://github.com/lukebemishprojects/Tempest/blob/1.20.1/forge/src/main/java/dev/lukebemish/tempest/impl/forge/WeatherDataProvider.java, but https://github.com/lukebemishprojects/Tempest/blob/1.20.1/common/src/main/java/dev/lukebemish/tempest/impl/data/world/WeatherChunkData.java is the attachee-aware bit. Technically yes I could pull everything out into static helper functions that take the data and the chunk and do stuff? But frankly seems kinda pointless. Is there a reason it's designed in such a way that the thing being attached to isn't a generic you can capture or anything?

Don't look at that mod too closely. It's got some cursed stuff

[raven coral]

Well it wouldn't be a generic, rather an object

[quaint yoke]

Is there a reason the API was designed in a way that the type is erased there?

[fleet gull]

https://github.com/Commoble/morered/blob/main/src/main/java/commoble/morered/wire_post/PostsInChunk.java

looking at the code, I really only use it to sync updates to players tracking the chunk

is automatic syncing of chunk capabilities a thing now?

[raven coral]

the alternative is to give the attachment type a generic

[raven coral]

not yet™️

[fleet gull]

ah okay so if I make it an AttachmentType<LevelChunk> then it works

[raven coral]

what

you need AttachmentType<PostsInChunk>

[quaint yoke]

I think you're misunderstanding me. Why does an attachment have to be attachable to anything?

Why can't we have attachee-specific attachments?

[raven coral]

my initial design had AttachmentType<target, data> but it was a bit messy

nobody says you have to make your attachment attachable to anything... after all you're attaching it 😛

[quaint yoke]

If it's not necessarily attachable to anything, it should be trivial to pass in a typed object it's being attached to. The only reason to ignore that is if you're making a contract that attachments have to be attachable to anything, was my perspective there

Basically: if I need unsafe casts to do that, that's indicative of a flawed design

[fleet gull]

well... it was trivial in the past because we had the AttachCapabilitiesEvent<T>

do we still have generic events?

[raven coral]

not anymore in forge

probably going to remove them from eventbus in 1.21

[quaint yoke]

Yeah generic events aren't a solution either

But this doesn't require that I can't think

[raven coral]

the problem is deserialization, luke

[quaint yoke]

Methods with type parameters within an event should suffice

[fleet gull]

ahhh I see where this went

previously, we'd construct and attach the thing, and then deserialize data into the thing, right?

[raven coral]

yep

[fleet gull]

and now we construct it on deserialization

[raven coral]

exactly

the new system is a lot more codec-friendly

[fleet gull]

tbh even without automatic syncing I could refactor my chunk data to not have a chunk reference

[raven coral]

that's what I was counting on initially, yeah

[brazen verge]

it's just that some implementations will be a pretty painful and gross refactor, for something that doesn't seem that difficult to fix on neo's end

like, we have the attachmentholder, why not provide it?

[raven coral]

    Test chunkAttachmentReferenceTest:
        FAILED - GameTest fail: class net.neoforged.neoforge.attachment.AttachmentHolder$AsField cannot be cast to class net.minecraft.world.level.chunk.LevelChunk

oops hehehe

[raven coral]

there it is: https://github.com/neoforged/NeoForge/pull/462

[quaint yoke]

Any reason AttachmentHolder can't be parameterized based on the thing its getting attached to? Like, where exactly does that cause difficulty? I'm assuming I've just missed something obvious here

AttachmentType woul have to be parameterized such too

But then the default value supplier could take specifically the type in question as an input

And you'd just need a recursive generic on IAttachmentHolder to tie it all together

[brazen verge]

Attachment holders can hold more than one object

[quaint yoke]

Yes that changes nothing I said

The generic I'm saying to add is the "what is being attahed to" one

[brazen verge]

I'm confused then

AttachmentType is already parameterised

oh wait I misunderstood

yeah I get you

[quaint yoke]

I'm proposing to get rid of the unchecked cast, basically

Because there's no reason it should need an unchecked cast

[raven coral]

I think it adds a lot of verbosity for a relatively infrequent use case

[quaint yoke]

It's really not that much verbosity

And it prevents attachment of stuff to unintended targets

[raven coral]

You need to have the target in every single attachment type, it's not great

• the self type for IAttachmentHolder is not particularly clean either

[quaint yoke]

Eh, the self type is pretty trivial

But in general, APIs should be typesafe unless there's a good reason why they shouldnt be

And frankly I'm not seeing a good reason here - a slight bit of added complexity is worth the type safety

With your setup, it's impossible to actually use that extra parameter in a typesafe way

[brazen verge]

Well you can just instanceof, but yeah

[raven coral]

The extra parameter is somewhat niche IMO

It's not perfect but I think it's better to have the cast than change the API again

I'd have to try to change it to see what the impact is 🤔

I expect every single attachment type declaration to need a change but it might be worth it

[brazen verge]

We’re still in beta tbf

[raven coral]

Yeah and it's better to change it now than later should we change it

The question is whether we want to change it at all. I really like the "first create the attachment type then attach it to whatever you want" approach

[radiant minnow]

Worth noting that if you want to be able to attach it to random stuff, you could just generify with the upper bound

[raven coral]

You wouldn't be able to attach to random stuff I think

[radiant minnow]

Why not?

Everything that supports attaching data extends a common interface, does it not?

[brazen verge]

Making the holder have a type itself doesn’t change anything functionally though

Its just allowed for a type specific object to be provided at attachment

Rather than a blind one

If anything, it helps support your intended implementation

Because it allows you to actually differentiate things at attachment

[raven coral]

the type needs to be part of the AttachmentType as well

so you'd have AttachmentType<Entity, MyData>

[brazen verge]

Oh right cus of the type erasure

Ehhhhhhh

I’m less of a fan of that then

I like the quick and easy typeless deployment

[raven coral]

me too

[brazen verge]

I’m changing my vote on that

But the original thing is still a necessity - but youve done that already so that’s solved

[raven coral]

Supplier<AttachmentType<ChunkMutableInt>> CHUNK_MUTABLE_INT = DR.register("chunk_mutable_int", () -> AttachmentType.serializable(chunk -> new ChunkMutableInt((LevelChunk) chunk, 0)).build());

vs

Supplier<AttachmentType<LevelChunk, ChunkMutableInt>> CHUNK_MUTABLE_INT = DR.register("chunk_mutable_int", () -> AttachmentType.serializable(LevelChunk.class, chunk -> new ChunkMutableInt(chunk, 0)).build());

[brazen verge]

I’m voting for keep as is

[raven coral]

noted

[radiant minnow]

Why that class there?

[young oyster]

No way to meet in the middle by making the .serializable method have a generic overload?

Or, like, .<LevelChunk>typedSerializable

[cedar oar]

it's not just serializable

[young oyster]

Well, yes

Just giving an example

[cedar oar]

hmmm is this a bad idea?

AttachmentType<Thing> remains as-is, no context.

a new class SpecializedAttachmentType<Thing, LevelChunk> would be used via builderSpecialized/serializableSpecialized(LevelChunk.class, chunk -> new MyThing(chunk)).build()

the context would always be provided into the AttachmentType, but by default AttachmentType would discard it when calling the lambda.

the reason to have SpecializedAttachmentType would be that this would enable a new overload of getData(SpecializedAttachmentType<?, T> where T would be the type of the owner LevelChunk implements AttachmentHolder<LevelChunk> or whatever

thanks to erasure, the additional generics would be binary and source compatible with any mod expecting there to not be a generic

waits for Tech's "oh god no..." response, despite not seeing anything wrong in the idea :P

[brazen verge]

my issue with it is it feels like an additional layer of complexity and debt for something that's ultimately just serving to be type-safe but only sometimes

[young oyster]

I'm down with that

Then again I'm a slut for type safety and doing stupid things with generics, so..

[quaint yoke]

Right but remember that you can use bounds. So to attach it to anything you'd have a bound of IAttachmentHolder or Object or whatever

Because you can attach it to anything extending the bound, not just only the bound

Heck, you could even do what giga suggested with that system pretty trivially

[raven coral]

😛

giga's idea vastly increases the API surface for little benefits

[golden silo]

Personal opinion: We should increase the API if that means we have proper compile time verification of consumers of the attached object

[raven coral]

We also need to keep the API approachable and limit overall complexity, it's a delicate tradeoff

[golden silo]

Sure

But not really here

The approach gigaherz showed above

Is completely approachable (we do the same in the Holder <-> DeferredHolder systems)

And the overall complexity is low for the same reason

As such the idea holds merit in my opinion

[raven coral]

It's a lot of added complexity actually, basically duplicating all of the user-facing API

[golden silo]

Given that one can extend the other

And that the SAT has a specific usecase, that is not really true

[raven coral]

there isn't really a reason to have both

[golden silo]

There is

Compatibility

[raven coral]

I'd rather do a BC than significantly duplicate the API

[golden silo]

Okey

Then do that

[raven coral]

I just discussed with Tslat that doing that might be worse than the status quo

[golden silo]

Okey, but I don't agree

There seems to be a significant usecase to be able to capture the object to which the Attachment is being made

[open hemlock]

I don't agree either. A more cohesive type safe API would be better than the status quo.

[golden silo]

And that should be part of the API
Which is something we all agree on

And based on good java programming

That means that it should be type safe

As such

The status quo is at least two orders of magnitude worse then the proposed solution

[raven coral]

status quo here means with the current PR accepted, i.e. with a cast

[golden silo]

Link?

And if that is not compile time safe

Then it is still an order of magnitude worse

[raven coral]

https://github.com/neoforged/NeoForge/pull/462

[golden silo]

Okey

I will read up and then form my opinion

[young oyster]

I feel that a semi-random cast has a much higher chance of being a footgun than a strong .class type reference... if you use a .class you are guaranteeing at compile time that you know the type/subtype being attached to.

People can syntax sugar their way into hiding the types if it looks "ugly" to them.

[raven coral]

many simple attachments do not need the class type at all; for example, any attachment using a codec directly cannot capture the target

[brazen verge]

that also effectively kills multi-type attachments

since your attachmenttype would have to be typed to a specific object

[quaint yoke]

As I pointed out before: this is false - #1135328752658829312 message

[brazen verge]

but then that's not type safe

which is the whole point of doing this change in the first place

[quaint yoke]

No you miss the point - it is type safe. If you have an attachment attachable to anything, you can't know what it's being attached to - so it's an Object. If it's being attached to only one sort of thing, you use that for the generic and thus it's typesafe

[raven coral]

Might be possible with enough generics (? super H)

[quaint yoke]

It really only needs one extra generic here or there. It's not as complicated as you all made it out to be, at least from my look through AttachmentType and AttachmentHolder

[brazen verge]

Its not its just added visual noise, which feels hurty on whats atm quite a clean and minimalist visual

But yeah I get it

Type safety is more important

[quaint yoke]

The way I see it, if we wanted to sacrifice type safety for a lack of visual noise we'd all be using groovy all the time (/hj)

I mean, I do use groovy sometimes, but sometimes I want type safety

[brazen verge]

lol

[raven coral]

Pretty much this

I really like the concise types and declarations

[young oyster]

Lol. Concise? In Java?

public record EmptyEdge<SD, S extends Node<SD>, TD, T extends Node<TD>>(WeakReference<S> source, WeakReference<T> target) 
  implements GraphEdge<S, T> { }

[quiet chasm]

when it gets that bad, you make a class that encompasses it

[brazen verge]

🙂

[brazen verge]

yeah but codec & DFU is cheating

lol

[cedar oar]

that's not codecs/dfu, that's my WIP codegen library

[brazen verge]

ah

[cedar oar]

wrote that mess in order to be able to define a method with varying number of type-safe parameters :P

(the idea for that lib was to be able to express the same you would in java source, not bytecode, so it deals with expressions and loops and such, instead of load/store/jump/...)

[raven coral]

I have had a shot at adding a generic: https://github.com/neoforged/NeoForge/pull/467/files

the self-type makes me extremely sad

the other drawback is that this only works if you only want to capture a rather general attachment type (for example Entity)... you'd still have to cast to Player @brazen verge

[stoic warren]

you missed write having a reference to the holder if that wasn't intended

[raven coral]

it was intended

if you have a reference to the holder it's already in the attachment

[quaint yoke]

Ah, yeah, no easy way around that limitation just with how java generics works unfortunately... sometimes I really wish I could tear apart the Java type system and redo it all

[golden silo]

Join C# we have all the generics goodies 😄

[noble rampart]

static interface method constraints ❤️ ❤️ ❤️

There is some really really good stuff in C#

[golden silo]

Extensions!

[noble rampart]

Yes, reified generics 😅 (which can be annoying for someone used to Javas generics, I'd have to say)

Source generators MVP!

[quaint yoke]

not the stuff I want. But then again what I want is basically type classes

In the functional sense

But yeah reified generics are sweet

[brazen verge]

yeah that's fine

[raven coral]

Thing is, I'd rather have the simple system than the limited yet way more complex second option 😛

[brazen verge]

agreed

it's a lot of extra visual noise and fundamental system complexity for something that ultimately provides only the bare minimum of 'safety' for a niche set of usages

like I'm not really even getting type safety out of this - I still have to instanceof and cast

and in return every single attachment for everyone has an extra generic param and stuff

as far as I'm concerned this generic doesn't even achieve what people seem to be arguing it does

at best you know that the object you're getting is a chunk or stack - beyond that you're type checking everything anyway

something I did think of though @raven coral

was - is there a way to have your attachment provider return a null?

like for example if I'm getting an object that I can't attach to (say client player vs server player or something)

and I don't want to attach anything to the client player

is it possible to implement a null-check that just doesn't attach if it returns that

[raven coral]

Ah so you'd have @Nullable X as the attachment type, and default to null?

[brazen verge]

I guess

I was more thinking odn't attach at all

like if I don't want to attach at the point of provider, don't

this would be useful for context-vague attachments

such as coming from a generic entrypoint

[raven coral]

Hmmm, what do you return when getData is called?

[brazen verge]

that's my question yeah

I guess maybe just make the data an optional

if so that's probably fine as-is

[raven coral]

I was thinking of the provider just throwing an exception tbh

[brazen verge]

that's also possible - since this should only ever happen when you're contextualy passing something you shouldn't have been

though I can't possibly foresee every edge case on that, it'd have to go to discussion further

although..

the provider wouldn't have to do that anyway

the person implementing the provider should be the one throwing the exception

so again fine as-is
maybe nothing needs to change after all lol, thanks ducky

[raven coral]

Pretty much yeah, IMO

Ducky?

[brazen verge]

Rubber ducky?

[raven coral]

Ahhh yes yes

[brazen verge]

Saw those two new roles in #announcements
Will either of those ping me when this gets merged?
I cant release until this is in lol

[stoic warren]

sure

[raven coral]

Did you read up yet?

[golden silo]

Yeah

I am struggling to make up my mind

Both have their advantages

And their disadvantages

In particular both struggle with the case where i for example know the exact type of (block) entity i am attaching to

And I want to capture the instance of that type

Neither api constructs seem to really support that scenario

Well not in a type safe compiler checked way at least

Do I have that correct?

[raven coral]

Yeah that's correct

[quaint yoke]

And unfortunately there's no easy way to do that with the way you've done attachment stuff, with the interface shoved onto the object

Perhaps a different system might work instead? Something that doesn't depend entirely on an injected interface to actually attach/retrieve data, but a static method instead, so the literal type of the thing being attached to is never erased?

[raven coral]

One could do ATTACHMENT.get(player)

[quaint yoke]

Exactly

And attachment could then have a very specific type

[raven coral]

It could be done

[quaint yoke]

It's cleaner than either current proposed option in my mind. Actually type safe for every use case, only one type (attachment type) to worry about and attachment holder is basically unneeded, etc

[raven coral]

You still need IAttachmentHolder actually, and the impl gets even trickier for chunks

[quaint yoke]

Many of the actual methods on it can be made into an implementation detail though

Youd have to extract some logic from attachmentholder to attachmenttype though, yeah

[raven coral]

It's worth considering that getting a backref to the holder is rather niche 😉

[brazen verge]

^

Theres nothing stopping people from implementing their own provider/wrapper checks if they want better safety too

The current implementation is super simple and clean which is excellent for usability

Hampering that for a niche use of a niche use seems.. ehh

[quaint yoke]

My point is that the API where the lookup is done from the type is cleaner anyways, with or without that backref. You don't depend on people having to interact with an attachmentholder at all that way - they only have to worry about the type (unless they're making some custom type implement IAttachmentHolder or something funky)

[quaint yoke]

Moving the lookup to the attachment type would be just as simple and clean

And you'd avoid any generic on IAttachmentHolder still

The only added generic would be the extra one on AttachmentType

And that generic could be super-specific too - you could make it Player

And it would Just Work™️

[raven coral]

How is X.getData(y) cleaner than y.getData(X)?

[severe axle]

object.getData(type) is typically more readable, as it indicates you are asking the object for stored data of type type

[quaint yoke]

But by definition it's less type safe

Because you're limited to the type of the dispatch, which is the root type of the holder

[raven coral]

it's a tradeoff

[quaint yoke]

It's really not much of a tradeoff

I don't see how moving that to the type complicates the API that much

It's basically the same - whether it's "more readable" one way or the other is entirely subjective cause they're basically identical - except that you can be more typesafe in literally every case

[raven coral]

I think the use case is sufficiently niche to not require perfect type safety

[quaint yoke]

Tech, what's the actual cost of moving it to the type?

Like, you're saying the use case is niche

But why not support it?

In what way does it complicate the API enough to not support it?

[brazen verge]

what would the registration of a data attachment look like after your proposed change

[quaint yoke]

I mean, you'd just do type.getData(holder) instead of holder.getData(type)

[brazen verge]

registration I mean

[quaint yoke]

And the actual constructor for the type would be basically identical, just with an extra generic for the holder type, which can be as specific or generic as you'd like

Sec, lemme write out an example

For an unserialized one (basically the same either way): ```java
private static final Supplier<AttachmentType<IAttachmentHolder, SomeCache>> SOME_CACHE = ATTACHMENT_TYPES.register(
"some_cache", () -> AttachmentType.builder(holder -> new SomeCache()).build()
);

Make an overload with a supplier and it'd be pretty similar

[brazen verge]

so basically you're still left with the bare minimum of type safety that you'd usually still have to cast anyway?

private static final Supplier<AttachmentType<Entity, SomeCache>> SOME_CACHE = ATTACHMENT_TYPES.register(
        "some_cache", () -> AttachmentType.builder(holder -> holder instanceof ServerPlayer pl ? new SomeCache(pl) : null).build()
);```

or would it accept any subclass of attachmentholder

[quaint yoke]

Any subclass

Because the method that gets the data from the object is on the type, not the holder

So it can have a specific type for the parameter it takes

[brazen verge]

which results in classcast exceptions that can't be managed by the user in the event of someone trying to retrieve data for a type that isn't theirs

I.E. someone trying to use a data attachment on a LivingEntity that's typed for a Player

and additionally is virtually unusable if obtaining the attachment type by id rather than having an object reference at compile time (for inter-mod work, etc)

[quaint yoke]

...well yeah, the data type it's attached to is part of the type. If it's meant for a Player, you can't use it on any old LivingEntity...

[quaint yoke]

If a modder feeds a data attachment type the wrong holder type... well obviously one would hope that's a class cast exception. It's a class cast exception if I unsafely cast to Player too

[quaint yoke]

It's not though? Just specify the type when creating the deferred holder same as anything else?

[raven coral]

• Everyone has to change their code again
• This would introduce an API difference with capabilities where the query does happen on the object
• You get a second annoying generic that makes the internal code less readable
• It adds a level of indirection because the attachment type needs to forward to the attachment holder internally
• You can't use a Supplier for the query, so it will probably look like DATA.get().get(entity) instead

[young oyster]

• Not necessarily true
• Explain more
• "annoying generic" = bias. I and others want type safety, and won't care about that.
• Internal detail? Most modders won't care.
• This can probably be worked around? Why is this?

[fleet gull]

personally I'm just going to rewrite my chunk cap datas to accept the chunk as a method arg

if I really wanted to keep myself from giving the wrong chunk to it then I could do like

private void doChunkStuff(LevelChunk chunk) {
  // do stuff with chunk and this.value
}

public static void doChunkStuff(LevelChunk chunk) {
  getChunkData(chunk).doChunkStuff(chunk);
}

[raven coral]

• changing from entity.getData(attachment) to attachment.get().get(entity) is quite the difference
• capabilities are queried on the holder (e.g. entity.getCapability(Capabilities.ItemHandler.ENTITY)), it's nice for symmetry with data attachments (entity.getData(ATTACHMENT))
• then let's agree to disagree. type safety is a means not an end
• it's important to keep the internals readable in case someone needs to read through them
• if you use DR you get a Supplier<AttachmentType<YourType>>, IAttachmentHolder currently has helper methods that also accept a supplier

[supple pollen]

I think entity.getData makes a lot more sense

[quaint yoke]

Welp, I guess I'll just avoid using data attachments then. About the only thing I needed them for was easy attachment to chunks and I can inject some stuff pretty easily to do that myself

[noble rampart]

So, wait. Chunks allow you to attach data. But you want to inject into bytecode instead to... attach data to chunks?

[quaint yoke]

I mean for my use case attachments give me literally no benefit if the data is unserialized, unless they're aware of the thing they're attached to in a typesafe way. If I'm already giving up on that... might as well go for a multiloader solution that's really not all that complicated and can easily be aware of its holder type

[fleet gull]

why does the data need a field reference to its own chunk though

couldn't you just pass the chunk in in method args

[mint current]

I mean, both proposed solutions for capturing the owner provide sufficient type safety for your use case, so your argument doesn't make much sense

[quaint yoke]

I could rewrite every method literally anywhere that deals with the data, yes. It would probably involve some needless API breaks but it's an MC version change anyways so, sure?

But... why? Why would I make needless changes to adapt to neoforge's bad design decisions?

[quaint yoke]

They actually don't. Last I checked the other proposed solution only would give me an AttachmentHolder

[fleet gull]

now I kinda wanna write a lib that's basically SaveData but for chunks

[golden silo]

I think personally the limiting factor is entities

[fleet gull]

because like with SaveData I can do this ```java
public static LevelCircuitManager getOrCreate(ServerLevel level)
{
return level.getDataStorage().computeIfAbsent(tag -> new LevelCircuitManager(level), () -> new LevelCircuitManager(level), ID);
}

[mint current]

I'm pretty sure both solutions allow you to at least get the basic type (i.e. level, chunk, entity, blockentity)

[golden silo]

This should really be possible with data attachments

If it is not

then I think that is a major flaw in the api

[quaint yoke]

That requires adding a generic param to the attachment type. Tech's proposed solution doesn't do that, at least last I checked

[golden silo]

No. none do

Both solutions only offer either object or IAttachmentHolder

[quaint yoke]

If you're adding a generic param to the attachment type, it's not really worth it unless you go the whole mile and switch it all over to the type

[quaint yoke]

...mine does though?

[golden silo]

Link, I only know of the two solutions provided by tech at the moment

[quaint yoke]

"proposed solution" doesn't mean "implemented solution" - my proposal is moving the query to the attachment type and adding a new generic to it for the attachable type

[golden silo]

Ah yeah no, that idea is not really great. Because the concept does not really work with the way DR's work

[fleet gull]

looking at this again, I think the important concept is that if it had persistent data, it would be a holder composed of two fields, one with the persisted data and one for the level reference

so the data still wouldn't have a field reference to the level

but, why should it need to? it's data

[quaint yoke]

...wut? In what way? You can easily have two generic types on something in a registry...

[golden silo]

That is kind of what I am thinking about

[mint current]

I did misremember that both solutions do it, but that's also false, one of them does provide the basic type: https://github.com/neoforged/NeoForge/pull/467

[quaint yoke]

Yes, I'm just saying extend this to provide the specific type so I'm not fucked when I do this with Player

Which is pretty dang easy

At least, if you're already adding the generic type it is

Which that proposal is already doing

[golden silo]

Then I would say: Make A PR

Or at least an example

[quaint yoke]

"we've told you several times that your idea is something we don't want... but we refuse to listen to you unless you put in the effort to make something that we've said we don't want"

[golden silo]

No I mean, I would like to see an API version that has proper type safety for this

Where you can actually get a Player when you add a data attachemtn to a player

[quaint yoke]

And I've shared examples of what usage would look like. I can try to make a PR anyways since it shouldn't be very difficult - but frankly it's super fucking low priority given the attitude towards it I've seen here

[noble rampart]

At least, if you're already adding the generic type it is
Big qualifier there, though

[quaint yoke]

Well yeah. But if you want type safety you have to add a generic somewhere... and it's really not like it complicates it that much

[brazen verge]

that's why I asked for an example up above

[timid cargo]

lukebemish

[Reference to](#1135328752658829312 message) #1135328752658829312 [➤ ](#1135328752658829312 message)For an unserialized one (basically the same either way): ```java
private static final Supplier<AttachmentType<IAttachmentHolder, SomeCache>> SOME_CACHE = ATTACHMENT_TYPES.register(
"some_cache", () -> AttachmentType.builder(holder -> new SomeCache()).build()
);

tslat referenced · Jan 8, 2024, 4:09 AM

[brazen verge]

with usage as such:

SomeCache data = MyDataAttachments.SOME_CACHE.get().get(player);

[raven coral]

with my preferred solution you only get IAttachmentHolder

[golden silo]

To be honest tech, object is equal to IATTachementHolder for useability here in my opinion

[raven coral]

with IAttachmentHolder you can at least look up the inheritance hierarchy of it

solution 1 (you get IAttachmentHolder): https://github.com/neoforged/NeoForge/pull/462
solution 2 (you get the base type): https://github.com/neoforged/NeoForge/pull/467

[brazen verge]

the original issue is that the current implementation doesn't give you anything

the whole thing that started this was I wanted some reference to the object so I could at least use it

now the debate is about type safety, which is where we're at currently

tech has PR'd his solution, but luke is unsatisfied with the type safety on it and is proposing the above change to the API

I personally prefer the hypersimplicity of tech's implementation, given how niche the need is for luke's thing and how it additionally makes every get significantly more gross looking

but I understand the arguments for explicit type safety as well

[golden silo]

In reality I don't really care, I will you experts decide here

Tech if you say go with sol 1 then we will do that

If you say go with sol 2 we will do that

[noble rampart]

I'll be honest, not using attachments, so I am not that attached to this discussion, and will detach myself from it.

[golden silo]

LOL

[brazen verge]

personally I think that the curren impl/design of attachments is really dev-friendly, which is historically something forge has really, really lacked in data attachment stuff

which is why I'm not a fan of luke's change, because it's a decent step back from that, which hurts to see when it's in such a good position atm

new devs have always struggled with understanding how to use/register caps, and we finally have an impl that fixes that

[noble rampart]

okay I do have $.02... I thought that data attachments are pure data. Why do they need to know about what they are attached to?

[brazen verge]

plus, MyAttachments.MY_ATTACHMENT.get().get(player) Is super gross lol

[brazen verge]

that was tech's original thought too

but there's plenty of edge cases that require fore-knowledge of what it's being attached to

[noble rampart]

They have no behavior on their own, so whenever they are retrieved by some behavior code, that code inherently knows what it is retrieving the data from 🤔

[brazen verge]

like what if my data is side-sensitive

[golden silo]

Yeah that was my understanding as well

Let me rephrase the question:

[brazen verge]

I don't have any understanding of sides at the time of data attachment registration

[golden silo]

Can we get the exact type when we are talking about the capability?

I think for Data attachments, personally it should not matter

[brazen verge]

ok lemme sum up

[golden silo]

They are just comparable to a map, which also has no idea in which it sits

But capabilities as something that is supposed to expose an API

Should have access to the exact type and instance it is attached to

[noble rampart]

The cap provider is a triple-generic, it is constrained

[buoyant patio]

Imo that is not a nice API

[brazen verge]

that's luke's suggestion

and I agree

[golden silo]

Indeed it is not

[brazen verge]

which is why I don't like it

[buoyant patio]

I think this whole discussion is devolving into "luke can't be arsed to write one additional line of code so the API is bad"

[raven coral]

In some cases, people also associated some behavior with that data

[raven coral]

Not really. He wants the type safety at all costs

[buoyant patio]

Which he gets with an instanceof

[brazen verge]

Current Implementation

Registration
public static final DeferredHolder<AttachmentType<?>, AttachmentType<Float>> ENERGY = register("energy", () -> AttachmentType.builder(() -> 0f).serialize(Codec.FLOAT).build());

Usage

float energy = player.getData(MyAttachments.ENERGY);

Issue

With no reference to the attached object, the data attacher cannot safely discriminate in situations where it needs to (I.E. side-safe code, type-relevant code, etc)

Proposed solution 1 (Tech):
Registration
public static final DeferredHolder<AttachmentType<?>, AttachmentType<Float>> ENERGY = register("energy", () -> AttachmentType.builder(holder -> 0f).serialize(Codec.FLOAT).build());

Usage

float energy = player.getData(MyAttachments.ENERGY);

Difference

The registration can optionally use a function instead of a supplier, receiving the IAttachmentHolder at attach time

Proposed Solution 2 (Luke)
private static final Supplier<AttachmentType<IAttachmentHolder, Float>> ENERGY = register(
"energy", () -> AttachmentType.builder(typedObj -> 0f).build()
);

Usage

float energy = MyAttachments.ENERGY.get().get(player);

Difference

Type is now a typed generic, allowing you to receive the exact object typing at time of attach. Introduces another generic placement, and changes how retrieval works

[buoyant patio]

Having it not have the generic also allows the same type to be attached to multiple different things

[brazen verge]

luke's argument to that was you can just type it as the root type you want (I.E. Object, etc)

[noble rampart]

With no reference to the attached object, the data attacher cannot safely discriminate in situations where it needs to (I.E. side-safe code, type-relevant code, etc)
Can you give an example?

[timid cargo]

technici4n

[Reference to](#1135328752658829312 message) #1135328752658829312 [➤ ](#1135328752658829312 message)Can you link me some of your capability implementations where you capture a reference to the holder?

tslat referenced · Jan 3, 2024, 9:48 AM

[noble rampart]

ah is this for returning... idk... initializing a data-attachment differently on client-side? but that's really only for the default-value provider, innit?

[quaint yoke]

No, the point is that I can't do anything if the instanceof fails. I have to throw or do an unsafe cast or crap

[brazen verge]

yes

[fleet gull]

I had another thought derived from my prior thought

The SaveData api is really nice

like, you don't need to register anything

[brazen verge]

for chunks

[fleet gull]

you just ask it to getorcreate your data and bam, data

[buoyant patio]

It is a clear no to lukes suggestion from me at least to the part of the retreval change

[raven coral]

That's why Level data attachments were not in the initial rework PR

But it turns out that they are also useful on the client side

[brazen verge]

^

[fleet gull]

I use SaveData on the client, yeah

[noble rampart]

hacksssssssssssssssss 🐍

[brazen verge]

Optional<> data?

[raven coral]

That's a bit gross too 😛

[brazen verge]

I don't disagree, but everything is gross here lol

[quaint yoke]

I mean I think my proposed solution is quite clean

[buoyant patio]

Or return a null default instance in that case (my point being the modder can decide)

[raven coral]

You're not supposed to return null 😛

[quaint yoke]

...they can decide with my proposed solution too?

[fleet gull]

public static MySaveData getOrCreate(Level level) {
  if (level instanceof ServerLevel serverLevel) {
    return etc;
  }
  else {
    return CLIENT_DUMMY;
  }
}

[noble rampart]

Okay, THAT is an honest to god good argument

[supple pollen]

If the AttachmentType is made raw, you can still cause a cast exception

[raven coral]

What you can do is make the attachment type a private variable in some class, and gate access to it with static methods that have stronger type bounds

[noble rampart]

But yes, that is the counter-argument

ask yourself: are you separating behavior from data

or are you ....

[raven coral]

Also for deserialization

[fleet gull]

like seriously I'd rather just have a savedata-like api instead of *waves hands around*

[raven coral]

Another example would be some ItemStack attachment whose properties depend on the Item

[brazen verge]

i mean that's kinda what it's like now

.get is inherently getOrCreate

[raven coral]

For example some fluid handler whose capacity depends on the Item of the stack

[brazen verge]

and set is auto-dirtying

[fleet gull]

yeah but why do I need to register anything

[raven coral]

So it can be deserialized

[brazen verge]

and used on the client

[fleet gull]

savedata can deserialize stuff, I don't need to register anything

[buoyant patio]

If it is on the wrong thing you should imo

[raven coral]

You're implicitly registering it when you pass the name

[noble rampart]

Okay BUT a) that only applies to the default provider, doesn't it? b) afterwards it's the responsibility of the code making use of the attachment, not the attachment itself

[brazen verge]

correct

but that's all we're discussing here

[raven coral]

It also applies to the deserializer

[brazen verge]

or meant to be anyway

[fleet gull]

right, I specify the serializer when call getOrCreate, which lazily loads it

[noble rampart]

okay, or that

[raven coral]

Unless you want to serialize the capacity instead of getting it from the item

[noble rampart]

but why does the type itself need the generic bound in that case? can't it be hidden?

[brazen verge]

yes

the fact that you're even debating the validity of it is why tech and I want it done via his method lol

[noble rampart]

sorry, I am just asking questions probably already answered 10 times in here 😄

[brazen verge]

it's so niche that the tiny bit of type safety involved really doesn't measure up to the cost of changing it

the cost being a whole other generic identifier and getter being a super gross supplier get get

[raven coral]

Btw if you need a specific Item class, having strong typing of the ItemStack isn't gonna help you

[quaint yoke]

Ya know, I can't be assed to fight this out. I've spent most of my time modding for forge working around extensively limiting API design, and I'll keep doing so it seems. Not really a big deal

[brazen verge]

I think realistically

we need some sort of semblence on what to do if we want to return a nothing, Tech

I.E. if luke's object typing doesn't match what he needs

if you're not happy with an Optional, then you need to be nullable

[raven coral]

IMO throw an exception

[quaint yoke]

...that's literally the point of the generic. It enforces that at compile time!

[brazen verge]

it also forces every single person who uses data to then have an extra generic and use supplier get to get the data

[noble rampart]

Trade-Offs, Trade-Offs 👀

[brazen verge]

for something so niche that feels like ap retty hefty tradeoff lol

[fleet gull]

why do I need any generics

[brazen verge]

you don't

[noble rampart]

use a static method and private your attachment-type to guard it

[buoyant patio]

But for the runtime validation you'd need the Class too

[brazen verge]

with tech's proposal

luke's specifically and explicitly requires it

[fleet gull]

SaveData doesn't have generics and I can still get a MySaveData

[raven coral]

IMO null is likely to cause a random NPE later down the line

I'd rather fail fast

[quaint yoke]

...wut? No... the generated bytecode for the lambda that takes the specific type contains a cast

[fleet gull]

and heck, I can shove a level reference in it too

[buoyant patio]

Which results in a class cast exception without a check

[quaint yoke]

1 generic - what sort of data you're attaching
2 generic - what you're attaching it to

And yeah, really... less necessary than it ought to be. This is as complicated as it is in part because of how the attachment API is designed, with all the registration stuff and whatnot.

[quaint yoke]

...yes. What else would it be? You're trying to get data from the wrong type of object. It should cast just as much as trying to treat an integer as a string

[fleet gull]

why do those need to be generic

okay lemme rephrase that

why does something need to be able to attached to more than one kind of thing

[brazen verge]

the whole thing is ironic given that currently you don't know what you're attaching to at all lol

[quaint yoke]

That's... not actually relevant? Even if it's attachable to only one thing, you still have to ecode that somewhere. Like, how else would I tell it that something is attachable to only a Player?

[brazen verge]

I just wanted a backref

[quaint yoke]

But yeah Commoble I think I see your point

You could remove both generics with something a lot more like SavedData

Grab a data storage from... anything, compute the thing you need

And in the actual computing method, just capture the outer thing outside the lambda if you need to

[fleet gull]

if you normalize your data properly so it doesn't reference the player holding it, you don't actually need to tell something that the data is only attachable to players

[quaint yoke]

Because no need to register shit ahead of time!

Like, if I were doing this with saveddata it would all be trivial

[raven coral]

Unfortunately that won't work for item stack comparisons

[fleet gull]

do we need to care about itemstacks

[quaint yoke]

Normalizing attachments that way is not always possible because they're not always just used for attaching pure data

[fleet gull]

maybe people should just use regular ol' itemstack nbt

[quaint yoke]

Or, well, it's possible but fucking painful many times

You basically end up writing stuff in a way where you'd really like some sort of monadic yield syntax that java just doesn't have if your stuff is sufficiently complex.

[brazen verge]

food for thought

data attachments don't need to be serializable

a lot of this is for volatile data

which saveddata is not for

[fleet gull]

I use savedata for volatile data

[quaint yoke]

But Commoble, that is actually a really good point - with something that looks like SavedData, not the registration based current API - you could remove both type parameters and still be fully typesafe!

[fleet gull]

like y'all keep being like

"savedata is nice but we can't use it for A because people want to use x and y and z" but I use x and y and z with savedata

[brazen verge]

I mean the API is in

I'm not sure arguing about completely demolishing it is the best approach here

how about we maybe just focus on the thing we're actually discussing

[quaint yoke]

Because you provide the lambdas when actually querying data with SavedData, not registering ahead of time - so they can capture (or not) whatever you'd like, and it'll be typesafe, and there's no type parameters to deal with

[quaint yoke]

Tslat this is the thing we're actually discussing

If a more saveddata-like API would improve attachments...

(Which I think it almost certainly would)

[fleet gull]

I'ma go make my own savedata api for chunks and use that instead of capabilities

Tallyho! to the mixins!

[cerulean kernel]

Is it possible to default-implement equals and hashCode on an interface?

[fleet gull]

I don't think so?

my hunch is that, since Object has concrete methods for equals and hashcode,

[cerulean kernel]

damn, that'd be nice because if we could do it on INbtSerializable, that'd let us sidestep the problem of itemstack comparisons

[brazen verge]

interface isn't object

[cerulean kernel]

[i.e. require attachments to implement value based equality semantics, default implement them based on serialization]

[fleet gull]

something that extends Object (i.e. anything) and implements YourInterface will have a diamond problem, so java will force the implementor to implement hashcode/equals manually

[cerulean kernel]

maybe registering an attachment could require an EqualityComparator?

[fleet gull]

anyway @quaint yoke if the api as it currently exists doesn't fit your use case or ergonomics then rolling your own data storage framework will probably be more productive than refactoring the api again

[quaint yoke]

Yeah I'm liking your SavedData approach. Might go implement something for that

[brazen verge]

alright cool, if that's the case then can we get @raven coral's thing merged so I can use it lol

[raven coral]

@brazen verge released in 20.4.81-beta

[brazen verge]

https://tenor.com/view/yes-clapping-applause-praise-gif-16442370

[raw hull]

no, no, no, no, please god no 😭
#squirrels-🦊 message

[raven coral]

Fabric now supports ProtoChunk attachments

Who wanted this, again?

You wanna PR that? 😛

[kind jay]

I kinda wanted it, not sure if I expressed it before, but I think it would be nice to have

wait, is it now my duty?

frick

[raven coral]

You don't have to

But I'm happy to review the PR rather than write it

[kind jay]

ok, I'll do the PR

[raw hull]

It seems it could just be moving the implementation from LevelChunk to ChunkAccess, and handling copying (from proto -> level), and impostors (should expose the level chunk underneath)

[kind jay]

and the serialization stuff

because that is handled somewhere else

[raw hull]

in ChunkSerializer, yes, but right now it's guarded by a instanceof check, essentially. It doesn't need to be.

[raven coral]

merged

[buoyant patio]

Topic: sync non item attachments

[young oyster]

New thread

[stoic warren]

what if we make you responsible of that

[buoyant patio]

what if NO

[buoyant patio]

well it is a point in Future work for attachments in the blog post so...

for entities it should be relatively easy to support

[quiet chasm]

I already made a branch for it like a week ago to start looking at solutions ¯_(ツ)_/¯

[serene minnow]

what if BlockEntityTag saved itemcapabilities?

[fleet gull]

what?

[serene minnow]

the location of the saved nbt data on the item

if you put it in the BlockEntityTag, that data will be copied to a BE when placing the item down

[quiet chasm]

sounds like you should just extend BlockItem to copy/remove the nbt over to that tag

it doesn't make sense to force that on every item attachment

[serene minnow]

setting it in on place is possible yes, but it's not that clean

[raven coral]

Why not place caps from it though

[raven coral]

I was initially waiting for the networking rework to do that

[serene minnow]

?

[raven coral]

If an item has data attachments in a specific tag we could load them on the BE

[serene minnow]

You would have to do that in the onPlace method of the blockitem though? As other stuff isn't copied into the BE. But either way I can live without it, just rewrote the itemstackfluidhandler to store the data in the tag.