#Capability rework

1 messages · Page 2 of 1

raven coral
#

I think it should be kept if we can make it pull nested generics, cause that's quite a nice feature

rain tartan
#

well, TypeToken works with Maps, and those have nested generics

raven coral
#

yes but the transformer impl is completely custom here

rain tartan
#

to the IDE! jumps through window

radiant minnow
#

It also pulls nested generics

#

At least, in theory

buoyant patio
#

is it possible to hide the creation of the anon class inside a method?

rain tartan
#

hmmm. now I'm not so sure that CapabilityToken works on nested generics

#

I think it only works on the outermost generic type, and only if that is a class

raven coral
#

the class must exist in your mod

#

let's introduce

public interface IAttachedCapabilityProvider<T> {
    @Nullable
    T getCapability();
}

for attached capabilities

#

the question is now what to do with the capability provider in IForgeItem#initCapabilities

#

oops forgot the direction context 😛

late iris
#

Requires the level to tick the calendar tho

#

(Not to revive the topic, just wanted to share)

buoyant patio
#

wait for what do we actually need the runtime generics?

raven coral
#

in theory you could have different capabilities for IHandler<ItemStack> and IHandler<FluidStack>, but they erase to the same thing so asking for a Class<?> object wouldn't work

buoyant patio
#

yea but if we use a registry the thing that is actually identifying it is the ResourceKey

raven coral
#

but is there any sort of guarantee that you don't mix two different types for the same id by mistake?

buoyant patio
#

you can't since an id can only be used once (and if you retrieve it using the wrong type it is user error anyway)

fleet gull
#

also we shouldn't be asking it for a Class<?> object, that's the whole point of resourcekeys

raven coral
#

are resource keys typed with the object type, or just the registry type?

#

idk, it's a bit weird to me

buoyant patio
#

the object type or any supertype that is still inheriting from the registry type

raven coral
#

so we'd have ResourceKey<Capability<?, ?>>?

#

also, there's nothing to register since Capability contains no information whatsoever

#

I'd rather have the creation of a Capability object count as "registration"

fleet gull
#

e.g. the id for ForgeCapabilities.ITEM_HANDLER would be ResourceKey<Capability<IItemHandler>>

raven coral
#

what would you even register?

fleet gull
#

the Capability

raven coral
#

can you write a quick demo/example for the item handler?

fleet gull
#

actually wait hmm

buoyant patio
fleet gull
#

the resourcekey and the Capability here are kind of being overlapping concerns

#

looking at vanilla analogues, normally the thing-being-registered is a behavior flywheel like Item or a serializer like codecs and recipe serializer

#

(or both, like Block)

quaint yoke
#

I... Really dislike the idea of using the capability type as some sort of key for the capability. As people should be able to register multiple capabilities that happen to have the same type. Anything else is, frankly, an absurd limitation

#

And if you don't have that, you'd really need something like a ResourceKey

#

...could just do that thing a couple of the vanilla registries do where they register their own location or whatever. Do they still do that?

#

Or, hmm, you'd need type info somewhere, so nevermind

raven coral
#

we could give a resource location to Capability

#

so even if two caps have the same types, they are not the same instance

fleet gull
#

that's what the resource key does

#

resource keys also hash faster than resourcelocations since they use ==

raven coral
#

the idea is that you should specify which caps you want to "listen" to in the attachment to speed up queries when there are a lot of attached providers

#

it doesn't even reduce boilerplate for cap providers

image.png
raven coral
#

then you'd compare the Capability objects using ==

raven coral
stoic warren
#

fun

rain tartan
#

i see those TODOs thinkies

raven coral
rain tartan
#

wait a second

#

this @MustBeInvokedByOverriders annotation. how new is it?

raven coral
#

20.0.0 of jb annotations, whatever that means

rain tartan
#

hmmm, I think last time I went poking at JB annotations package (for fun), t'was 19.0.0

#

makes sense
as you were

raven coral
#

next up on the todo list is syncing and copying dead_tater

rain tartan
#

good luck kek

raven coral
#

surely there is a non-disgusting way to do it

rain tartan
#

~ Technici4n, moments before disaster

raven coral
#

for itemstacks it's quite easy: always sync all the caps by adding a special nbt subtag

#

unfortunately nbt serialization to packets sucks

#

are there other cases than PlayerClone where entity data gets copied to another entity?

raven coral
#

teleport seems to be handled correctly

rain tartan
#

end portal in the end (leading to overworld)

raven coral
#

seems like the player just gets removed and then revived, so that should just work™️

#

but maybe it doesn't 🤔

cerulean kernel
#

There's an argument for doing it in LivingConversionEvent as well [e.g. zombie becomes drowned]

raven coral
#

oh god that is something that needs to be handled too help_me

cerulean kernel
#

I don't know if anyone currently does, because it's pretty obscure

#

i.e. people making capability data that applies to villagers might not even consider what happens if their villager turns into a zombie and back

#

and they might not even attach their capability to zombie villagers

raven coral
#

I'm thinking about an overridable

default void copyCaps(ICapabilityProvider target, CopyReason reason)
{
    if (this instanceof INBTSerializable && target instanceof INBTSerializable)
        ((INBTSerializable<Tag>) target).deserializeNBT(((INBTSerializable<Tag>) target).serializeNBT());
}
#

for items we might force the copy every time without checking this method

cerulean kernel
#

ICapabilityProvider target, is this the attached capability or the entity?

#

wait i guess it could be both

raven coral
#

attached capability, but yes it's not perfectly clear

cerulean kernel
#

wait, entities don't implement INBTSerializable

raven coral
#

there will be some sort of IAttachedCapabilityProvider, so that would be the parameter type - and also where the method would go

cerulean kernel
#

honestly, the fact that 'capability provider' both refers to the thing involved in attachment and also the big five "object what can have caps" classes is a source of confusion in these discussions [though idk if it causes any problems for actually coding mods]

raven coral
#

in fact I pushed some version of IAttachedCapabilityProvider but I'm not super convinced right now

#

maybe we should rename CapabilityProvider (the class) to something else - CapabilityHolder or something

raven coral
#

maybe we should just allow normal ICapabilityProviders to be attached with a list of capabilities

#

so on top of doing just event.addCapability(name, provider) you'd also have the option to do event.addCapability(name, provider, CAP1, CAP2, CAP3)

#

the latter should be preferred for performance reasons, but the former will still work (especially for providers that don't know the full capability set in advance)

noble rampart
cerulean kernel
#

does this support multiple attached capabilities providing the same capability, conditionally on either the context or some internal state?

#

[it doesn't make much sense for side context or for 'disabling' one cap to fall through to the next, but a couple of the use cases around string/resloc contexts need it]

raven coral
#

Yes multiple attached providers can provide the same capability

cerulean kernel
#

so iirc Lex's position on this had always been that using a map for this is not actually faster until you reach some unreasonably large number of attached providers

#

I don't know if anyone's ever actually measured it.

#

[on the other hand, there's no reason we couldn't implement a map class that uses linear search for a small number of entries and hash for a large number]

quaint yoke
#

If we really are worried about speed I can write up a cursed solution that makes it all field accesses instead

#

But honestly, an IdentityHashMap is not much slower than array lookups, even if it is slower

#

(a normal HashMap could be slow however due to calculating hashes)

fleet gull
#

but yeah you don't want to use keys with slow hashcode/equals

cerulean kernel
#

a slow equals would absolutely murder the list approach anyway

fleet gull
#

ah right

cerulean kernel
#

and you wouldn't override hashcode with identity equals

#

i can imagine hashcode being slower than equals, though, I guess

fleet gull
#

by "identity hashing" I mean "objects that don't override hashcode/equals", not IdentityHashMap

cerulean kernel
#

if equals is comparing a long sequence and can short circuit e.g.

raven coral
#

IdentityHashMap should be even faster than a regular HashMap

quaint yoke
#

Not by too much if the stuff in the regular hash map didn't override equals/hashCode, but definitely still faster. I'm not super worried about the performance impacts here; if they end up being an actual issue, we'll figure that out then

#

...and if they end up being an actual issue I think arrays or lists aren't a good solution either and we should just do cursed nonsense so it's all field gets

raven coral
#

They don't store things in the same way

#

Doesn't HashMap always force a pointer indirection due to the linked list

quaint yoke
#

Hmm, I'd have to see where in the source that is

#

I'd assumed an IdentifyHashMap was just a hash map that used the identity has code and identity equality instead of hashCode and equals

#

But I could definitely be mistaken

cerulean kernel
#

it might be marginally faster because it doesn't have to do a virtual method lookup

#

.....oh, that's clever

#

i'm not sure i trust it, what if a key object being queried happens to be in the table as a value at the position it expects it in

proven wigeon
#

Since we are talking about capabilities could forge add item handler to bundles?

quaint yoke
#

Oh, that should definitely be a thing

#

Neoforge already does it for shulker boxes so it can't be that hard

raven coral
#

It's not that simple, there's some design decisions to consider

quaint yoke
#

I mean, yes. There's some work that would go into it. But it's better that forge offers that than 5 different mods all trying to add it themselves in subtly different ways because forge doesn't

proven wigeon
quaint yoke
#

Yes

raven coral
#

They're not a stable feature so I wouldn't really worry about them

proven wigeon
#

waaaaaaa . Only reason they don't have a recipe is because pocked edito 💀

quaint yoke
#

Given that plenty of users, mods or modpacks end up messing with them - I don't think their experimental-ness is enough of a reason for forge not to support stuff

proven wigeon
#

Yeah I would like for itemhandler to be there also because it would be a nice thing to copy if one were to make a similar item

raven coral
#

Do people actually do that? That seems like a bad move but heh

#

(I mean force-enabling bundles)

supple pollen
#

Reference2ReferenceOpenHashMap may be faster than IdentityHashMap

quaint yoke
cerulean kernel
#

actually implementing an item handler for bundles is challenging too because they don't have slots

proven wigeon
#

Could make an interface that superclass itemhandler for "inventory without slots" clueless

quaint yoke
#

I do hate how the current item handler interface works

#

Item transfer shouldn't assume that slots exist

#

You should be able to insert items to... Wherever the thing wants to put them, list items, and extract items given an item type

#

Actually exposing slots should be optional

#

(though obviously there's still reasons some inventories would want to do that if it made sense)

cerulean kernel
# 
interface IItemHandler {
    List<ItemStack> insertItems(List<ItemStack>, simulate)
    Iterable<ItemStack> extractItems(ItemStackPredicate, maxCount, maxStacks, simulate)
    Iterable<ItemStack> enumerateAllContents()
}
    

interface IItemHandlerSlotted {
    default impls of all of the above
    all methods of current IItemHandler
}
quaint yoke
#

Yeah, except I'd say IItemHandlerSlotted should instead expose List<IItemHandler> getSlots()

cerulean kernel
#

not sure about that one

quaint yoke
#

(and/or possibly IItemHandler getSlot(int slot))

#

Basically, interactions with one slot are just interaction with an item handler that only exposes that slot

cerulean kernel
#

anyway aiui one of the reasons slots are so important is because mods want to be able to cache what slot an item is in so that they don't end up doing the linear search that these default impls would necessarily do on every operation

#

i don't know if allowing a "slot" to implement the interface I proposed [i.e. may itself contain multiple stacks of items] is a good idea or not for that

#

IItemHandlerModifiable.setStackInSlot is also important for item handlers that can be used in gui menus

quaint yoke
#

Ah. Hmm. So maybe List<ItemSlot> getSlots() where that interface exposes slot specific versions of the stuff on ItemHandler

cerulean kernel
#

each slot object could have a single ItemStack field instead of messing with NonNullLists

#

the List<ItemSlot> would be an immutable list, created either when the handler is constructed or on the first call to getSlots

quaint yoke
#

(I say this because I have something that deals with this exact use case)

cerulean kernel
quaint yoke
#

Ah, yeah. That makes sense

cerulean kernel
#

InvWrapper would call Container.getContainerSize each time it's called, and invalidate if the size has changed

quaint yoke
#

Could just do getSlotCount and getSlot(int slot) instead

#

That way it's impossible for anyone to even try to modify the list

#

Even if it's backed by one internally

cerulean kernel
#

...what if you just implemented List<Slot> directly on the cap

quaint yoke
#

Oh god. Uh. That seems funky. But, like... Maybe?

cerulean kernel
#

er not Slot, Slot is for guis

#

but whatever class we use for the slot access

quaint yoke
#

ItemSlot or whatever

#

I do think that wouldn't be great as people might think they can add to the list or set it or whatnot

severe axle
#

Don't think making the BE one override only is a good idea - the BE lookup isn't free

cerulean kernel
#

...maybe "redesigning IItemHandler" (or redesigning the standard capability interfaces in general) should be a different thread

#

it's not really relevant to reworking how capabilities get attached

quaint yoke
cerulean kernel
#

Item.getCapability(ItemStack, Capability, Direction)
Block.getCapability(Level, BlockPos, Capability, Direction)

#

...do we want to be able to attach capabilities [with the understanding that they definitely can't have storage, they just need to provide a way to get an interface implementation for a given stack / pos] to Items and Blocks? those could be done in CommonSetup rather than an attach event that runs millions of times

severe axle
cerulean kernel
#

how about EntityTypes

severe axle
#

the flyweight is useful for precomputing attachments

#

or doing attachments, etc

quaint yoke
severe axle
#

Yeah, but then perf issues

cerulean kernel
#

the flyweight lookup concept has the problem of managing lifetime

#

like you definitely don't want to construct a new capability implementation every tick, possibly many times per tick, even if it's lightweight

#

....

#

honestly maybe what we need is a "shadow block entity" for blocks that don't have block entities

quaint yoke
#

I would say you separate API and data lookups, and make it safe to cache the API until the flyweight at that context changes

quaint yoke
cerulean kernel
#

it'd be a perfect solution to the composter problem

#

you wouldn't put one at every pos in a chunk, just the ones that actually need them

#

but they could exist and not be synced [since you can't give a vanilla client a block entity that doesn't exist in vanilla]

quaint yoke
#

What happens if someone wants to add a capability to basically every block out there?

#

Basically, my idea is that all, say, MyBlockWithEntity would provide a single capability provider that can get instances of the capability. Those instances which you get from that are safe to cache

#

Now you're not doing a lookup every tick

cerulean kernel
#

how do you make them safe to cache

quaint yoke
#

Just a check of whether the flyweight has changed

quaint yoke
cerulean kernel
#

but how do you invalidate them when i break my composter

quaint yoke
#

Just a check of whether the flyweight has changed

cerulean kernel
#

also

quaint yoke
#

Same as the caching you can do with BEs now

cerulean kernel
#

how do you stop them from being constructed every tick when something does do a lookup every tick

fleet gull
cerulean kernel
#

like, is this a forge managed cache?

quaint yoke
fleet gull
#

MyApi.get(level).getThing(pos)

don't need capabilities for that

quaint yoke
#

Or the end consumer

quaint yoke
fleet gull
#

???

#

I already have a working mod that does this and lets people implement it for their own stuff without using caps or hard dependencies

#

just loads impls from jsons and assigns them to blocks in a Map<Block, Thing>

quaint yoke
#

I have an API I want to expose about blocks. I want to add default impls to a lot of vanilla blocks, but also let people implement their own providers for their blocks

fleet gull
#

yeah datapacks are good for that

quaint yoke
#

That's literally what capabilities are made for

quaint yoke
fleet gull
#

yes I can, I already did

quaint yoke
#

Not in the sort of cases I'm thinking of

#

I'm not talking about some mod of yours here. I'm thinking of a system I could easily build that, if you, like, added "shadow" BEs to blocks to make stuff work with capabilities, would cause a heck ton of BEs to suddenly exist

#

Fundamentally, cap lookup should be flyweight based. I can think of ways to cache stuff just fine with this system still

fleet gull
#

yeah I'm not talking about apis that can be supported by different types of things (blocks, items, etc)

quaint yoke
#

Sure. I'm not really sure why you wouldn't want to base cap providers on flyweights. You've for just as much of a caching issue with block entities as with the flyweights, it's just based on a different object

fleet gull
#

I am attaching things to blocks

#

but just blocks, not items

quaint yoke
#

Sure. I was not talking about your specific case; feel free to just use a map for that

#

If you can get it to work with datapacks and a simple map, no reason to use capabilities I guess

#

But there's plenty of cases you would want caps on lots of vanilla blocks, and/or on flyweights in general, and I think limiting them to BEs is a bit counterproductive

severe axle
raven coral
#

But it may not be necessary, I agree

#

Flyweights don't have state, but what if you need state

severe axle
#

honestly the shadow BE concept sounds interesting for block caps, just return a fake BE which is only good for cap access

Just need to figure out how to mark those as removed when the block changes (though I guess the block handles that)

raven coral
#

Wtf

#

No that sounds awful

quaint yoke
buoyant patio
#

block caps would be easier after the split since the whole point for them is to not have their own data but expose things the block has (like fluid in a cauldron)

quaint yoke
#

Like, I'm confused - why can people not cache a CapabilitySupplier or something for a given level/pos that has a method that checks whether the flyweight at that location has changed, and if it has updates it's cached capability instance, and otherwise keeps the existing one?

#

Like, the chain would go flyweight -> capability provider + level + pos -> capability supplier -> capability, with that second to last step skipped if you didn't care about caching

raven coral
#

Did you look at my commit?

severe axle
#

because you have to keep doing level.getX(pos) checks

#

those are not free

raven coral
#

They're free if you cache them

#

And the API will provide a caching system for block entities, I just haven't written it yet

quaint yoke
#

But the important bit is that using flyweights as keys here isn't going to make caching impossible or something

#

And solves a ton of other issues

raven coral
#

I don't really see what it solves

#

Forget about data and API split

#

Cause that adds more complexity than it removes, IMO

quaint yoke
# raven coral I don't really see what it solves

Flyweights being the lookup bit instead of instances? It means you don't need instances in order to have caps. Which means, basically, you don't need BEs for caps, which currently causes a ton of limitations

surreal frost
#

here is a benchmark with FastUtil Vs Java/HPPC/EclipseCollection.

Though it compares how much faster it is with Primitive types.
Which makes it a bit biased against java collections.
https://github.com/Speiger/Primitive-Collections-Benchmarks/blob/master/BENCHMARKS.md

And that uses JMH.

GitHub
Primitive-Collections-Benchmarks/BENCHMARKS.md at master · Speiger/...

Benchmarks for Primitive Collections. Contribute to Speiger/Primitive-Collections-Benchmarks development by creating an account on GitHub.

#

also @raven coral I expect you to provide a way to make a capability cache that DOESN'T require a Constant revalidation of Data.
Because all these: "Are you valid" checks cost a TON of performance over time.
IMO yes forges capability system needs rework but the changeability they provide gives a major performance boost because you are not constantly reevaluating if your data is correct.

If you provide a internal solution that does that automatically I honestly wouldn't care but it should be something like this:

public void doLogic(CapabilityCache<IItemHandler> cache, Direction side) {
  IItemHandler handler = cache.get(side);
  if(handler == null) return;
  //DO LOGIC.
}

Assuming you test blindly, the cache should also provide information which side for example is present.

(Its just the BlockEntities that I personally care about requiring a cache, because that has the most interaction over all mods shared)

But you defined in your post that something like that should be provided.

about if it for "Optional" or "not Optional".
Honestly if the Optional doesn't provide anyhting extra like listeners it should not be present. Its just another Object Allocation that especially with BlockEntities throws garbage at the wall that Mojang already does tons of which doesn't help performance.
(Lazy optional if cached actually did the middle ground there xD)

#

The rest of it IMO is fine, even the ItemStack Capabilties no longer being able to store random data but should store into NBT is fine IMO.

Though I highly suggest you provide a way to NOT make NBT sync?
Why?
Dunno: maybe someone simulates a nuclear reactor inside a inventory and that has 25 item slots in there that change every single game tick and doesn't want to be synced to reduce network traffic. I dunno about you but a few KB of data sync 20/s because you HAVE TO GO THROUGH NBT isn't a good thing either.
Its like sending gui data every single tick to the client.

Also luckily nobody in the entire world has datacaps on Home internet, right? xD

Just a point you should keep in mind. xD

severe axle
#

not syncing itemstack data is a non-starter unless someone rewrites the entire creative menu

surreal frost
severe axle
#

The creative menu forcibly syncs itemstacks client to server, voiding any data only present on the server

#

so unless that is fixed, itemstacks need to be fully synced, always

surreal frost
#

Assuming its a dedicated area that isn't intentionally synced, that should be accounted for then by the person creating the NBTTags inside said area.

severe axle
#

you can't account for it

#

if someone switches to creative mode, your data will be voided

surreal frost
#

No i mean that the dev should account for "empty state" to be present. If it switches from "full state" to "empty state" it should treat it as if the item was freshly created.

surreal frost
severe axle
#

no, voiding people's information because they switch to creative mode is unacceptable

surreal frost
surreal frost
#

anyways the two things that Capabilities bring should be kept:

  • Support for caches/No validation required internally. (Callbacks are a good thing, especially if you have thousands of checks be done 20 times a tick)
  • Unsynced data for ItemStacks that can still persist to reduce network traffic for weak connection speeds & and datacaps. I know enough people that have 1 or two said cases xD
#

pops

quaint yoke
#

Yes, the game should account for NBT changes, sure

#

But the point is that is unrelated to the issue here

#

Let's say you have an ItemStack with server only unsynced information

#

The player goes into creative mode, opens their inventory, move stuff around

#

That unsynced data is now gone

#

And there's basically nothing that isn't exceptionally painful that can be done to avoid that

#

If you don't want to sync ItemStack data, don't store it on the ItemStack

#

Because the creative menu will just erase it if you do

quaint yoke
#

Even if the game can "adapt" and not crash - that doesn't change the fact that the data is now gone

#

...wait did they seriously come here, give uninformed feedback, and leave the server?

severe axle
#

FeelsSpeiger

noble rampart
#

Is it possible to only sync it fully in creative mode? Not really, right? One would have to resync the entire inventory (and possibly UI) when switching to creative then 🤔

severe axle
#

Yeah, and because items in the player inventory tick, you end up having to sync the entire player inventory every tick while in creative

noble rampart
#

Hmmm why is that?

severe axle
#

you can't just sync it on open, because the items could change over time while the menu is open

#

and even if you're constantly syncing, you can still get data loss due to network latency

#

because the creative menu just sends a packet with whatever the client contents was at the time

#

idk why they keep it implemented this way tbh

noble rampart
#

Well doesn't it sync automatically anyway as any other menu?

#

Oh wait, the actual creative tabs

#

Hm, you are right, but even if you always sync fully you can get data loss

severe axle
#

only for time-varying data, but yeah

noble rampart
#

I just meant that you conditionally serialize itemstack NBT fully or partially based on the players gamemode, and let the standard syncing take care of it.

#

Not fun to patch, though.

#

It might actually be easier to patch the creative menu instead lol

quaint yoke
#

Except then you'd probably break vanilla clients/servers somehow. Or, like, server-only caps attached to vanilla objects would break with vanilla clients or some weird bug or another... Simplest, in my opinion, is to always sync and to design item behavior to need to do as little as possible in a given tick

#

(like, store when a counter started instead of storing the counter or the like)

severe axle
#

Yeah, you just have to be careful with desgining item data

#

ultimately it has to be nbt-backed and translated from real data to nbt at some point

#

the best pattern appears to be sparse updating via onChanged mechanisms

raw hull
#

Another possible solution, though, is that on Forge/Forge connections, rewrite creative sync to be S->C. On a Forge/vanilla connection, assume any caps on a forge server won't exist on the client anyway, and can't be relied upon.

but that is also saying "rewrite how creative mode works in forge/forge connections" which is it's own can of worms.

quaint yoke
#

And also yeah, quite the can of worms

raven coral
#

I think always syncing is by far the easiest solution ^^

cerulean kernel
#

(or when they move an item around in the creative menu)

#

i wonder if we could do some sort of system where item stacks in the player inventory on the client have a uuid in the nbt

cerulean kernel
quaint yoke
cerulean kernel
#

the creative menu is difficult to rewrite to do S->C sync, it wouldn't be just a few patches

quaint yoke
#

If size is a worry, then syncing is by far not the slowest bit there

cerulean kernel
#

like, as-is, there is no server-side creative menu, it's only a client side screen and packets that have nothing to do with the menu system

quaint yoke
#

Because otherwise this will probably keep haunting the capability system forever

cerulean kernel
#

honestly at that point we might as well make a "forge creative menu", designed from scratch, and also e.g. replace tabs with a better system

quaint yoke
#

Sounds like it could be a cool mod

cerulean kernel
#

like, the creative menu has a usability problem in modded separate from the cap issue

quaint yoke
#

Yes-ish. Part of that is how modders use it

#

But honestly, a full replacement of that seems out of scope

#

Like, I don't want neoforge messing with my creative menu when I'm playing a nearly vanilla modpack

cerulean kernel
#

what if

#

hmm

quaint yoke
#

Is there any real issue with just making ItemStack data always synced?

cerulean kernel
#

what if we ship a sample implementation of an item handler for itemstacks that stores data off-stack in world saved data, the way sophisticated backpacks do

cerulean kernel
quaint yoke
#

Cause stacks get recreated too often

#

And can get nested, and whatnot

#

And that gets bad quickly

cerulean kernel
#

honestly we could probably do a system that capability-adding mods can opt into so that there's a single uuid shared between all attached caps to lookup all off-stack data, rather than one per cap

quaint yoke
#

The only correct solution in my mind is to tell people to store their big data off stack

#

And then sync everything

cerulean kernel
#

i think we need to do better than "tell" them

quaint yoke
#

But yeah, such a uuid system might be helpful? I dunno

cerulean kernel
#

i think we need a framework for off stack data

#

we'd need some sort of event or method hook or something to handle stack splits

#

and merges

#

which could be useful anyway

quaint yoke
#

I honestly am not sure it needs that much of a framework, but I guess I can see the benefit? I'm not sure how common this is but yeah, that could work

cerulean kernel
#

[splits more than merges, we can just refuse to merge stacks that aren't identical]

quaint yoke
quaint yoke
#

(different UUID that is)

cerulean kernel
#

maybe it's something an item would have to opt into

quaint yoke
#

Though really, do you know of any mods storing data like this on stacks with stack sizes of more than one?

cerulean kernel
#

if you attach a capability to an item that is stackable and need to handle splits specially, those items and only those items call a hook when split

quaint yoke
#

Also, there's no way to do splits safely

#

Because there's no one way to split ItemStacks

cerulean kernel
#

......ugh i just realized there's another problem

quaint yoke
#

But honestly, unless this is an actual issue that is being run into I think this isn't a big deal. A system to help set up off ItemStack UUID keyed data - sure. But events dealing with splitting or merging would be hell and honestly, not at all feasible

cerulean kernel
#

if we have off stack data indexed by a uuid, then that creates a potential memory leak when the stack is destroyed

quaint yoke
#

How does sophisticated backpacks deal with it?

cerulean kernel
#

i think there's a method that gets called sometimes when an itemstack is destroyed, but not reliably

#

haven't looked, hang on

quaint yoke
#

Even then, it's no worse than vanilla maps, right?

#

Like, vanilla already does this

#

So I'm not super worried

#

How does vanilla store written book NBT? Is that actually stored in the stack?

cerulean kernel
#

it removes the backpack data for a backpack worn by a mob if the mob dies without dropping it, it doesn't appear to detect any other cases of the backpack item being destroyed

#

written book nbt is in the stack yes

#

that's how chunkbans work

quaint yoke
#

Gah, lovely. Honestly, though, I wouldn't really worry. It's not like that many items with crazy amounts of NBT are going to be lost in a way where it doesn't fall into one of the ways of detecting that

cerulean kernel
#

honestly, 'leaking' could be a feature

#

it allows server admins to resurrect your backpack if you dropped it in lava by accident

quaint yoke
#

Yep. How many mods, currently, would be affected by just saying all ItemStack data is synced anyways?

#

Like, how many have this sort of large data to begin with?

cerulean kernel
#

I mean

quaint yoke
#

(where the data isn't needed on the client already)

cerulean kernel
#

Sophisticated Backpacks stores its data off stack, but other backpack mods don't

#

like, that's always been the big use case for non synced capability data

#

since an item like a backpack might have 27 or 108 or however many itemstacks inside, any one of which could itself be pushing the limit for the amount of data reasonable to store in one itemstack

quaint yoke
#

Like, you'll want it for the menu, eventually. Or to render an overlay of what's in the backpack. Or something

cerulean kernel
#

I mean, as a concept, most backpack mods only need it on the client when you have opened the backpack's GUI

#

and menu sync doesn't sync the whole list at once, it syncs one slot per packet

quaint yoke
#

Lots of people like being able to see what's in backpack without opening it, and there's mods that implement that feature for vanilla stuff for a reason

#

But heck, vanilla syncs shulker boxes

cerulean kernel
#

same with the overlay, you might broadcast the couple of itemstacks that are in, like, the tool slot or the sleeping bag slot, but not the full contents

quaint yoke
#

Maybe? Have you seen those shulker box view mods though?

#

They just display the whole thing

cerulean kernel
#

that's because shulkers sync their whole contents anyway

quaint yoke
#

Sure. I'm saying that backpacks should because it means people can access the item handler capability on the client and have the view be accurate

cerulean kernel
#

anyway, 27 slots is less of a problem than modded backpacks with a large number of slots, and you're not always going to have, like, a backpack full of bees

#

sophisticated backpacks does have a content tooltip, i'm not sure a general content tooltip that displays contents for any itemstack item handler is in scope

quaint yoke
#

I'm not saying it is

#

But I'm saying a mod could provide that

cerulean kernel
#

[also shulker boxes have a content tooltip in vanilla - the fancy ones have to remove it]

#

which creates a precedent that any mod that does this should be responsible for their own tooltip handling

quaint yoke
#

Eh, whatever. That's for mods to fight out honestly

cerulean kernel
#

also, in principle if we had a framework for off-stack data we could also have a framework for the client to request it

quaint yoke
#

But my main point is that I'm not sure there's cases where there's enough data that syncing is an actual problem, but it's not accessed or set up in a way that a sophisticated backpacks like system couldn't work

cerulean kernel
#

[and splitting it up if it's too big is easier if it's packets we control]

#

or maybe we just bite the bullet and autosplit all packets

#

i think there's been resistance to doing that in the past instead of just targeting specific packets that are known for growing too large

quaint yoke
#

In a modded context? That might not be a bad idea

cerulean kernel
#

but anyone might put an itemstack in a packet and not realize that itemstack is full of bees

raven coral
#

ok so this works except for one thing

image.png
#

the client can't differentiate between no nbt and empty nbt once it removes the _ForgeCaps tag

severe axle
#

could patch the comparator to make null and empty equivalent

#

honestly it should be already, but vanilla doesn't encounter this case

raven coral
#

no definitely not

severe axle
#

why? In what case is that desireable behavior?

cedar oar
#

but we can always do like _WasEmptyTag = true

#

and if missing / false make null

raven coral
#

yeah, something like this:

image.png
cedar oar
#

although

#

I was thinking

#

we know if the other side is "vanilla" or not

#

or rather, we know if the other side is a new enough version of forge

#

we can check the presence and version of network channels

#

(some existing features do this)

#

so in that sense, it may make more sense to do instead ```
if new_enough_forge: Send {
tag: <original tag> | null
caps: serialized caps
}
else Send <original tag>

severe axle
#

I wouldn't bother with a forge version check there tbh, just the vanilla or not

raven coral
#

we can send cap tags to vanilla too!

#

if they ever make it back to the server due to the creative menu, the cap will be deserialized correctly on the server

#

feature™️

cedar oar
#

thinkies true

raven coral
#

next up we have copying for entities

raven coral
#

yeah the separate interface makes things harder for no good reason

raven coral
#

you lose the ability to check if a capability is registered, and you need to specify a resource location for your cap, but in exchange you gain:

  • simpler registration
  • different capabilities can use the same capability class (this allows mods to define their own separate IEnergyStorage "ecosystem", for example)
  • no ASM hacks anymore (need to be removed from FML)
  • the doors are now open for capability-dependent context
brazen verge
#

Why map and not registry?

raven coral
#

what do you register?

#

the capability is already close to a ResourceKey, that's why it follows a similar pattern

severe axle
#

then why not just use ResourceKey and ditch Capability

brazen verge
#

^

#

Also, not a fan of calling methods get when theyre actually a compute

raven coral
#

because the context parameter is coming in the next commit

severe axle
#

the context is per-capability?

raven coral
#

spoiler alert

image.png
raven coral
severe axle
#

why would the context be per capability?

#

it should be per-provider

#

per-capability is going to create this nauseating headache where you cannot share capabilities for any reason since you will need one with a context type on a per-provider basis anyway

rain tartan
mint current
# severe axle why would the context be per capability?

Per-provider only really works for blocks/BEs, everything else can't be generalised to one type (unless you go for generic but comparatively expensive to compare stuff like a string) without significantly limiting what you can do with the API

severe axle
#

an interned string (resource key context?) is probably sufficient for entities if you require such arbitrary things

raven coral
#

I think picking a single context type for the entire provider is too limiting

raven coral
#

you can use an unsided item handler for both item stacks and entities, for example

raven coral
#

another thing is that we'll be able to remove IFluidHandlerItem and instead provide the stack setter as capability context (but that will be for another time)

radiant minnow
raven coral
#

would it lead to a better API?

#

(I don't think it would)

buoyant patio
#

are the classes actually used or are they just for determining the type

raven coral
#

they're used to validate that 2 different CapabilityTypes don't share the same id

rain tartan
#

is the type class or the context class exposed to the public?

buoyant patio
#

yea that could lead to problems

raven coral
#

it's also useful for some edge cases (e.g. resolving a direction from a different kind of context)

#

question: what should the nullability be for the context?

#

I kinda want

@ParametersAreNonnullByDefault
public interface ICapabilityProvider
{
    /**
     * Retrieves the Optional handler for the capability requested on the specific side.
     * The return value <strong>CAN</strong> be the same for multiple faces.
     *
     * @param cap     The capability to check
     * @param context The extra query context. <strong>CAN BE NULL</string> or not depending on the capability.
     * @return The requested capability, or {@code null} if it could not be found.
     */
    @Nullable
    <T, C> T getCapability(CapabilityKey<T, C> cap, @UnknownNullability C context);

but you kinda have to copy/paste the unknown nullability annotation for every override

#

another problem is that you're kinda forced to cast the context yourself, which is error-prone

#

I guess the context could be a sort of ModelData object where you can pass different keys and the provider chooses which one is the most relevant

#

yeah ModelData and ModelProperty would work quite well... 😛

golden silo
#

But the API surface would become very cluttered

#

I already hate it in the BakedModel API we currently have

raven coral
#

isn't it a matter of replacing @Nullable Direction side by CapabilityContext ctx?

golden silo
#

And then querrying: ctx.get(Side)

#

As well as building the context

#

On every access

#

So -.-

vocal trout
#

Minor thing tech but your javadoc up above has a small issue for reference
<strong>CAN BE NULL</string>
string != strong

golden silo
#

Yeah I personally feal that a weird thing.

raven coral
#

I was thinking we'd cache a context for directions and provide a bouncer in ICapabilityProvider

golden silo
#

Well I think it should just be @NotNull

#

And just have a Nullable like context type (Void or Unit)

#

For those which do not have contexts

raven coral
# 
    @Nullable
    <T> T getCapability(CapabilityKey<T> cap, CapabilityContext ctx);

    @ApiStatus.NonExtendable
    @Nullable
    default <T> T getCapability(CapabilityKey<T> cap) {
        return getCapability(cap, CapabilityContext.EMPTY);
    }

    @ApiStatus.NonExtendable
    @Nullable
    default <T> T getCapability(CapabilityKey<T> cap, @Nullable Direction side) {
        return getCapability(cap, side == null ? CapabilityContext.EMPTY : DIRECTION_CONTEXTS[side.get3DDataValue()]);
    }
golden silo
# 
    @ApiStatus.NonExtendable
    @Nullable
    default <T> T getCapability(CapabilityKey<T>cap) {
        return getCapability(cap, side == null ? CapabilityContext.EMPTY : DIRECTION_CONTEXTS[side.get3DDataValue()]);
    }
#

That is what I don't like.....

raven coral
#

Unit is a good solution assuming we keep a generic context parameter

#

but how do you express null direction with that?

golden silo
#

I would say that the context for an inventory should probably not be a Direction then

#

But like an "Attached Side" concept

#

Which obviously has all the usual directions

#

Plus an internal property on the neum

#

And then a nice method that converts the Direction into a side

#

Which checks for null

#

Given that it is an enum

#

With a static method

raven coral
#

so kind of like enum BlockSide { UP, DOWN, NORTH, SOUTH, EAST, WEST, INTERNAL }

golden silo
#

That should be pretty much the fastest way

#

Yeah

raven coral
#

would the context be a property of the holder object (so always "BlockSide" for blocks, always Unit for levels, etc...), or a property of the capability type

golden silo
#

Well that I don't know 😄

#

For me it is more a property of the provider

#

A given block entity

#

Could have many attached providers

#

Which can provide different combinations of capability type + context

#

They are in my eyes not limited to the holder object

raven coral
#

but the query happens through the holder object

golden silo
#

..... Well yes, but the actual provider is not the holder object.....

#

So its api could just be an Object

raven coral
#

I'd say if you can provide combinations of capability type (T) and context type (say C) then both T and C should be a property of the capability type?

golden silo
#

What the actual type is is not relevant to the world/blockentity/block/entity/itemstack

golden silo
#

Consider the following situation

#

We have a capability for an inventory

#

By default the block entity would provide it for sides

#

But now I want to attached a provider that does not use the side as context

#

But for example the player that is opening a container

#

So for me

#

The type T of the capability key

#

Is not directly attached to the type of the context

raven coral
#

hmmm, would providers perform an instanceof check on the context then?

golden silo
#

But the actuall provider is the piece in the chain that combines both the requested capability key + plus the value and type of the context

golden silo
# raven coral hmmm, would providers perform an instanceof check on the context then?

That would be a naive way of thinking about it.

You could build a generic architecture (somehow) where the consumer API on levels, block entity, blocks, entities, itemstacks, takes a key<T> and an object

While the provider has a generic version of the API, which takes a key<T> and a context <C>.

A system in between could handle looking up the right provider that can handle both.

#

Potentially precomputing a map

#

In such a way that you could do something like this:

#

attachmentEvent.attach(key, ServerPlayer.class, (blockEntity, player) -> new PlayerItemHandler(blockEntity, player));

#

While the BE itself would have a callback

#

Which would allow it to register the same kind of callbacks

#

Meaning that no direct implementation of interface

#

Or overriding a getCapability(...) and then doing a bunch of instance of and if-switch checks.

raven coral
golden silo
raven coral
#

Say I want to attach an item handler for horizontal sides only, how would that look?

golden silo
#

Hmm that is a good question.......

#

The question becomes

#

Do we want to support, fixed contextual providers

raven coral
#

I thought about an approach like that where you'd have per-block, per-item, etc registrations, but it ends up being a lot more verbose than the current API

golden silo
#

Where the provider knows exactly ahead of time what the possible value of the context would be

raven coral
#

I think it's nice to give some flexibility... Though in theory you could register the same provider for multiple contexts

golden silo
#

I think I discussed this once with ama

#

I never liked the getCapabilitystyle API

raven coral
#

Same as enforcing a specific capability for attached providers VS letting them choose which ones they limit themselves to

golden silo
#

At least not on the providers

#

Where they had to override it

#

Obviously on the consumer it is fine

raven coral
#

Well my original plan was to go for something closer to Fabric's API lookup which has that property that you describe, but it is a lot more API surface and that felt somehow worse

golden silo
#

The Fabric API Lookup is a bit weird I agree

#

But the core concept is not entirely wrong

#

Especially on the provider side

#

However I like the getCapability(...) api style on the consumer very much

#

blockEntity.getCapability(IItemHandler.KEY, serverPlayer)

#

Just feels right

raven coral
#

hmmm yes

golden silo
#

The problem with attachable capabilities, is that unless you want to create a new key for each combination of CapType + Context

#

the compile time safety gets shredded

raven coral
#

well I tried that and it sucks... and you might check the wrong capability type by mistake....

image.png
golden silo
#

Cause only at runtime you know what the total combinations for any given capability holder are

#

Which is why I suggested leaving out the checks entirely!

#

Move the invocation of the actual provider away from the blockentity/level

#

To something generic in the middel

#

And consider the blockentity/level on the same architectural position as the attach event

#

Aka simply something that providers a set of capability providers to a capability manager

#

Wether that "simply something" in the end also holds an API surface to get a capability instance from said manager is a whole different story

#

And should be something you address seperatly

#

Aka try first designing an API that works well for providers

#

And then try to design one for Consumers

#

And adapt where needed

raven coral
#

well, API Lookup works well for providers imo, however it doesn't handle data storage

#

it gets nastier when the attached provider wants to both expose a capability and store some data

golden silo
raven coral
#

with API Lookup the registered providers are all stateless

golden silo
#

They are two different concepts

#

There might even be several providers that access the same datastore

golden silo
raven coral
#

right, splitting API lookup and data attachment

#

I did sort of abandon that approach because the two systems need a lot of things in common, and they both require quite some API surface

#

but assuming we had a DataKey I assume you'd have up to a single attached data instance per data key

#

still, you'd need most of the CapabilityProvider and CapabilityDispatcher stuff, and on top of that we'd have to design an API lookup system

golden silo
#

That is true

#

But you have a potential N to 1 relationship between capabilitykeys and datakeys

raven coral
#

yes

#

but you'd probably still need about 80% of the current capability API just for data attachment

golden silo
#

Yes and No

#

They would for sure feel very similar

#

But the internal implementation of the manager

#

Would be significantly different

raven coral
#

yes

#

however it's a lot of new code that doesn't seem to bring a lot of benefits to me

golden silo
#

And personally I think it is good that the APIs for both subsystems feel very familiar

golden silo
#

He had this kind of API ready

#

And it looked very nice

raven coral
#

I already did; he designed his API based on mine

golden silo
#

And worked very well

#

Well supposedly not then

#

Cause you just specified that it would be a lot of new code

raven coral
#

oh, I meant inside of Forge; writing it is not the biggest problem

golden silo
#

We are not MCF where the apis need to be small

#

Personal opinion: I want a nice API, that is useable

raven coral
#

my problem is just that data attachment already gets you 80% of the way for API lookup

golden silo
#

That feels good

#

I don't care how much code needs to get written for it

golden silo
#

Maybe you can refactor it

#

So that it feels like less duplicate code

raven coral
#

hmmm

golden silo
#

I would say good is the enemy of perfect

raven coral
#

@viral latch can you publish your draft somewhere

golden silo
#

Make it first

#

And then tweak it

raven coral
#

well currently I've been tweaking the existing caps system to make it "better"

#

most issues can be fixed with some targeted changes

golden silo
#

I would say rewrite it....

#

Personal opinion

#

The current status

#

With lazy optionals

#

Nullable context

#

Weird attachment events

#

Performance issues

quaint yoke
#

Yeah, I think this is a "burn it down and start fresh" sorta deal here

golden silo
#

It just feels to much like a patchwork of hotfixes on hot fixes

#

And it feels like it has an underlying systemic issue

raven coral
#

some of these things can be fixed in a reasonable way

#

the API query part shouldn't be very had to design - it is just refactoring API lookup a bit to make the registration happen in an event

#

data attachment has more open questions because I don't like any of the alternative systems either

#

having to design data attachment "from scratch" was what motivated me to try to salvage the existing system first

#

but assuming we can find a good design for it then splitting both systems is definitely the way to go

#

so I'll ask some simple questions first and we'll see how far that gets us:

  1. do the attached data instances need to have a backreference to their container? (e.g. reference to holder entity)
  2. do you need to be able to outright replace the data instance? or would it be "final? (i.e. would you be able to do both entity.getData(DATA_KEY) and entity.setData(DATA_KEY, data)?)
#

point 1. might be useful to e.g. automatically call setChanged on the holder block entity if the data changes - otherwise it has to be done manually by the API caller; arguably that is not too much to ask for the caller since this is also how block entity data generally works

#

if instances do not need a backreference, the design can be very clean - final-ness doesn't matter that much however

cerulean kernel
#

has anyone thought about the questions i had a while back about syncing level caps to the client?

raven coral
#

I don't think we need to sync them

proven wigeon
#

Can we have synced ones for block entities?

raven coral
#

maybe using a system similar to stacks

proven wigeon
#

Syncing caps is my main gripe about the current system tbh

raven coral
#

how slow can this be 🤔

image.png
severe axle
#

this, somehow, looks worse than the current one

raven coral
#

the current one does exactly this, it's just hidden in a CompoundTag equals 😛

vocal trout
#

I've seen worse

raven coral
#

basically you get(DATA_TYPE) the data on the object

#

if the data exists you get it; if the data doesn't exist it loads a copy of the default instance and you get it

#

everything else is handled automatically™️

severe axle
raven coral
# 
public interface IAttachmentHolder
{
    boolean hasData(AttachmentType<?> type);
    <T> T getData(AttachmentType<T> type);
    <T> void setData(AttachmentType<T> type, T data);
}

(everything non-null)

#

idk, the serialization makes it 100% impossible to get wrong

severe axle
#

yeah but it's guaranteed to be slower than a true comparison

raw hull
#

implement comparator and provide a default which serializes? (or rather, make it explicitly opt-in to have a comparator)

severe axle
#

and as we're aware, stack comparison is very often

raven coral
#

it's not as bad as one might think - most stacks have very few serializable caps attached to them

quaint yoke
#

Stack comparison also often just uses the built thing that uses NBT, and we're not going to be able to force mods to use some other one easily, right?

severe axle
#

we just patch the check into that one

quaint yoke
#

Though that is important. If I compare too stacks by NBT, I need to be sure that I'm validly comparing their caps too

severe axle
#

thats where the check is currently

raven coral
#

I like the default idea

severe axle
#

you only need it for serializable itemstack caps

#

nothing else needs a comparator

#

as they are not comparable

raven coral
#

of course

raw hull
#

yeah, I just mean "new AttachmentType" if you don't pass in a comparator, or call .withComparator, it uses a serialization-based one behind the scenes.

raven coral
#

the attachment type will use a builder soon but I just wanted to get a proof-of-concept working as quickly as possible

quaint yoke
#

So, to clarify - ItemStack data holders will serialize/sync on write, right?

#

That fixes the syncing issues

raven coral
#

wdym by "on write"?

quaint yoke
#

They'll just compare based on a faster non-NBT implementation

quaint yoke
raven coral
#

no

#

they only serialize when the item stack is being saved or sent over the network

quaint yoke
#

Because otherwise what do we do about the creative menu issue?

severe axle
#

the important part of having the comparator is that otherwise you are serializing every tick for all stacks in an open container

raven coral
severe axle
#

and anything else doing per-tick contents changed logic

quaint yoke
#

Also, like, keep in mind that if someone tries to inspect the NBT, the data in the cap has to be visible there. This has caused pain with the current system where that's often not the case

raven coral
#

but I agree; the comparator will default to serialization, but will be overrideable

raven coral
#

if you really need to inspect the full NBT you'll have to call serializeAttachments on the stack

raw hull
# quaint yoke Right, but it's synced when modified, right?

there's a level of indirection between "it gets modified" and "synced". see: in that example, handler.insertItem will not cause it to save/sync, but the next tick in the container, it will sync because vanilla will notice the stack is != to the cached one and sync it again.

severe axle
#

I've seen areCapsCompatible dominate the isSameItemSameTags call in most profiles over the past couple years, but I don't have any on-hand anymore

raven coral
#

isn't it because of the event dispatch

quaint yoke
#

Cause you can't easily dump caps with a command, but whatever

raven coral
#

?

quaint yoke
#

Normally you can inspect NBT data with commands, yes?

raven coral
#

you can dump them by dumping the nbt

#

you just need to make your own command

#

or actually, you will see them if you dump the itemstack

quaint yoke
raven coral
#

since they do get saved when writing to NBT, just not as stack NBT but rather as a separate ForgeCaps field

raw hull
#

I mean it's at worst, exactly the same functionality as the current system

quaint yoke
raven coral
#

you have to be more precise... I don't really understand what you're trying to say here

#

what's the problem with storing cap nbt in a separate field of the item stack nbt representation?

quaint yoke
#

Hmm. Alright, that's probably alright, though I'd have to check what the commands in vanilla actually look at

raven coral
#

fyi this is the itemstack serialization code

image.png
quaint yoke
#

Basically, I want to make sure that commands will be able to inspect this but as long as they can it's fine

quaint yoke
raven coral
#

they give the full stack

#

otherwise you wouldn't see count and item id easily

quaint yoke
# raven coral they give the full stack

So, can I have a rundown of what this structure looks like again? The data attachment data is stored parallel to the item ID and item count? How does that work with /giveing an item with data on it? Or the copy_nbt item modifier?

#

Basically, is it:```
item

  • ID
  • count
  • normal item NBT
    --- stuff
  • Data Attachment NBT
    --- stuff
Or is the data attachment NBT somehow nested in the normal item NBT?
#

Because if it's not, you've just removed half of the datapack features relating to items that Minecraft has

raven coral
#

[22:32:31] [Server thread/INFO] [ne.mi.de.wo.it.ItemCapabilitySyncTest/]: ItemCapabilitySyncTest passed the brand new attachment stuff works!

raven coral
quaint yoke
#

That... Is an issue then

raven coral
#

this is exactly the same as the current production forge system, and I don't remember seeing an issue for that

#

in theory we could encode caps as a subtag like we do for network syncing

quaint yoke
#

It was probably overshadowed by the slew of other capability issues

raven coral
#

in fact it would use the same manipulation hooks

quaint yoke
#

Though I suppose I could be missing something obvious? But the built in item modifiers all work with NBT specifically

#

That is, the tag NBT of the structure

raven coral
#

yeah then they probably don't work with caps

quaint yoke
#

That is not too great

raven coral
#

I added a todo in one of the patches, for now it's a bit too early to worry about that

quaint yoke
#

I'll try to actually look at the code in vanilla later to see where it does look

#

And see how hard of an issue this would be to solve

raven coral
#

ok now time to take a stab at the API query part

#

should we have different capability classes depending on the target

#

so you'd have BlockCapability<T, C>, ItemCapability<T, C>, etc

#

that doesn't sound too good

#

however we need a place to store the block capability providers, the item capability providers, etc

cerulean kernel
#

/give definitely doesn't work with caps

#

encoding caps as a subtag raises the same question it did with the client sync - how do you differentiate between empty and null actual tags on a tag that has only cap tags

raven coral
#

that's a solved problem already

#

what should the registration lifecycle be? 🤔

#

we need to post an event at some point

raven coral
hushed ledge
stoic warren
#

nothing prevents you from using a codec, but they're a lot of bloat for something that might be called every tick

raven coral
#

I'll add a helper to use a codec

raven coral
#

itemstack capabilities and data attachment are both working now

raven coral
#

now the question is what exactly we should do with the context type

#

I really think it makes the most sense to have it in the capability, but which capability you should use in a given context might be confusing

#

e.g. it would be easy to use the unsided item handler with block entities by mistake for example

#

I feel like we kinda want BlockCapability<T, C>, ItemCapability<T, C>, etc...

#

we only need block, item, and entity caps anyway... chunks and levels only need data attachment

#

@golden silo thoughts?

golden silo
#

For now I don't care

#

As I said

raven coral
#

basically I write a full nice proposal and you will judge it based on how the API feels to use?

golden silo
#

I think it is the eventual provider that determines what the combination of context and capability type it is

golden silo
#

I find "feel" weird to describe

#

And it holds not much weight in the eventual process

#

But the reviewing is a subjective process

#

So in the end "feel" is important yeah

raven coral
#

yeah I see

raven coral
golden silo
#

Consider this

#

Well maybe

#

I don't know.....

#

I think it works

raven coral
#

hmmm, what works?

golden silo
#

The linking of the capabilitykey to both the type of the cap and the context

#

And you are right

#

The API feels better if both C and T are bound

raven coral
#

back to feels xD

#

but ok that works for me; going to continue in that direction

#

thanks

raven coral
#

should we keep storing all the wrapper IItemHandlers in the various blockentity/entity subclasses?

#

let's try without for now 🤔

#

in theory that's all we need to support Container and friends

image.png
supple pollen
#

Does this work with composters, whose containers only accept one item? The impl locks itself once it receives an item.

raven coral
#

that might require an entirely custom impl indeed

supple pollen
#

Generally I assume WorldlyContainerHolders expect their getContainer method to be called for every transaction but optimizing this using the intrinsics of those implementations might be worthwhile (or even necessary, depending on the API)

raven coral
#

hmmm, there's only a single WorldlyContainerHolder so it's hard to say how they should work

supple pollen
#

Composters are the only one?

raven coral
#

yes

supple pollen
#

What if mods implement the interface? They will likely look at the composter implementation and deduce that the method will be called for every transaction

raven coral
#

idk, they should use IItemHandler

supple pollen
#

Yes of course

raven coral
#

hmmm, hoppers have a custom itemhandler wrapper impl

#

and shulkers too

supple pollen
#

A mod still could implement it. There are three options then.

  • Crash the game if this is done.
  • Do not support the behavior; effectively ignore the interface. Potentially log a warning.
  • Support the behavior fully. Potentially log a warning.
raven coral
#

I would prefer to minimize the amount of custom implementations that need to be written

#

so imo querying for every single operation is a reasonable default

supple pollen
#

True. If this causes performance issues, that is on the mod author to fix by using the item handler instead of the container holder.

raven coral
#

something like this should™️ work

image.png
buoyant patio
raven coral
#

so it checks canPlaceItemThroughFace which InvWrapper does not call

buoyant patio
#

but why up and not side?

raven coral
#

the actual side doesn't matter

#

as long as it's not null it works

raven coral
#

so here are all the entity item handlers that Forge provides

image.png
cerulean kernel
#

uh where's that

#

you can't do horses that way

#

(well, more specifically you can't do donkeys and llamas that way)

#

the handler needs to be recreated when a chest is added or removed - i thought horse.getInventory was private

#

(also i very much dislike the hand/armor facing thing, but that's a different discussion)

#

wait when is this lambda called exactly? are these entity instanceof checks redundant when we could be attaching a different lambda to each entity?

raven coral
#

the lambdas are called every time getCapability is called

cerulean kernel
#

ok so why not something like

raven coral
#

I just ported the existing Forge code

cerulean kernel
# 
event.registerAll(ITEM_HANDLER_ENTITY, entity -> {
    if(entity instanceof AbstractHorse)
        return (entity, facing) -> new InvWrapper((AbstractHorse)entity.getInventory());
    if(entity instanceof AbstractMinecartContainer || entity instanceof ChestBoat)
        return (entity, facing) -> new InvWrapper((Container)entity);
    ...
});```
raven coral
#

no that's quite unclear

#
  • it doesn't work with how capabilities works
#

it has to get an EntityType not an entity

cerulean kernel
#

what

#

i'm confused now

#

you attach to an entity right?

#

like, this would be attaching the first lambda to an entity type, which runs when the entity is constructed and attaches the second lambda to the entity itself

#

so that calling entity.getCapability doesn't check "is this a horse, is this a minecart" every damn time it is called

raven coral
#

nothing runs when the entity is constructed

cerulean kernel
#

why not

raven coral
#

it runs when the entity is queried for a capability

#

this is pure lookup, attachment is a separate API

cerulean kernel
#

....yes i am saying that what runs should be a lambda attached to the entity

#

....what?

#

call it attachment, call it registration, call it whatever

#

but a horse knows it is a horse, a non-horse should not be asking if it is a horse on every call

raven coral
cerulean kernel
#

why

raven coral
#

to separate api queries and data attachment

cerulean kernel
#

ok but

#

that is not a reason to make api queries not attach to the entity

raven coral
#

of course it is; what is there to even attach

cerulean kernel
#

THE LAMBDA

raven coral
#

if you want the best query performance you register via the entity type

#

this is just a quick port for what was in forge before

#

but it might just be fast enough

buoyant patio
raven coral
#

why 🤔

buoyant patio
#

if it is dead the only thing you could care about is data not api

raven coral
#

good point

#

consider it done 🫡

cerulean kernel
#

yeah, people's clone methods or whatever should be using the data directly

#

god i wish EntityType had a Class<T> field

#

can we add that somehow? there's got to be a transformer we could use to do it to the vanilla entity types

raven coral
#

if you can create a fake level you can instantiate the entity type

buoyant patio
cerulean kernel
#

yes someone could use Entity.class and register their entity type as an EntityType<Entity> if they wanted to

#

i suspect most people would not

buoyant patio
#

the factory is always a method reference in vanilla so you could probably extract the class somehow
(except for the player which uses a lambda that returns null)

mint current
raven coral
#

is getHealth that slow

#

well I guess it doesn't matter that much for API queries

#

the spammy uses should be data queries, which shouldn't have that isAlive check

mint current
raven coral
#

oh, that's lame

#

I guess it doesn't help that it was getting checked by a ton of capability providers

#

now it should only be checked a single time per capability query, and never for data queries

late iris
#

Oh I can answer this

#

The lock shouldn’t exist doesn’t need to

#

We could patch it out even, sodium does iirc

raven coral
#

I guess you just make it a ConcurrentHashMap?

late iris
#

We made a no-op lock

#

It’s crazy because it was legitimately blowing up spark reports for us

raven coral
#

it's all on the main thread isn't it

late iris
#

Yes iirc

raven coral
proven wigeon
late iris
#

It could yeah.

#

But it’s not up to me obviously 😛

cerulean kernel
#

forge could probably just patch it out rather than replacing the lock object, but same concept

raven coral
#

replacing the lock has a lower patch surface 😛

proven wigeon
#

Didnt know synced data was that slow and caching in other variables sucks. If this is indeed an optimization should be added so performance goes brr

late iris
#

It’s bad because synced entity data is a great system for implementing stuff you want synced on your entities, and you use it rendering, goals, everywhere, so it slows down the client and the server with locking

mint current
#

Technically you should even be able to replace the int2object map with an array but you'd have to be able to know the required size in advance to avoid allocating a 255 element array for every entity when that size is rarely needed

cerulean kernel
#

you know the size at the time the entity is constructed, don't you?

#

though

late iris
#

I think we learned a few days ago that people add to the synched data list for vanilla mobs

cerulean kernel
#

using an array would interfere with my idea on syncing data accessor ids

late iris
#

Besides list is literally for this purpose

#

Or map

cerulean kernel
#

since it requires using ids outside the normal range for one sided accessors

mint current
cerulean kernel
#

what?

#

that makes no sense

#

like, i know you'd have to compute it, but all the information needed to compute it was determined during startup, someone's not going to randomly register a new entity data accessor at runtime

raven coral
stoic warren
#

ah yes 3k diff

raven coral
cerulean kernel
#

changing it to an array has a large patch surface though, and a list can't be that much better

mint current
#

An arraylist is almost definitely better than a hashmap

#

Especially when the IDs are almost guaranteed to be contiguous

proven wigeon
cerulean kernel
raven coral
#

let's get back on track

cerulean kernel
#

we could implement a custom map that's backed by an array for the 'normal' range i guess

raven coral
cerulean kernel
#

....maybe a separate thread for synced data fixes

buoyant patio
#

with just a patch that EntityFactory extends Serializable and then extracting the class from the method reference

raven coral
#

Let's not do that kind of thing. It's also a PITA for multiloader devs for no good reason

raven coral
#

To anyone passing by: don't hesitate to read the updated PR

severe axle
raven coral
#

Heh cool nobody's gonna review it 😛

#

I guess it needs extensive example code

stoic warren
#

send a link, PR needs a pin in here

raven coral
stoic warren
brazen verge
#

definitely gonna need some examples lol

#

I would very much appreciate this not being pushed until we've had a solid look at example usage and stuff

#

the way capabilities get used is one of my biggest gripes with the system

raven coral
#

Oh it will definitely not be merged anytime soon

#

But if I can get some eyes on it I appreciate it... Otherwise I am just working alone in my little corner 😄

raven coral
brazen verge
#

yes the way lol

raven coral
#

😦

#

I get the meme but idk what your point was 😅

brazen verge
#

all good, I'll wait for examples

raven coral
#

Tomorrow:)

mint current
#

Looks interesting at a brief skim. Assuming I'm understanding it correctly that the attachment stuff is intended to be the data storage, I have one hope: that we can finally nuke the stupid "persistant data" NBT tags in BE and Entity in favor of that

severe axle
#

idk about the one on BE but I use the one on entity for random misc things a lot

surreal frost
#

well the issue with Sophisticated storages approach is that you can not really simulate inserting multiple things into it without actually doing it and then reverting it because when you copy the stack the "attachment data/NBTData" does not get cloned as well.
This leads to dupe/void bugs if you don't want to blindly fill or accept anything into slots.
Also it would lead to not being able to trust that a "copy" is actually a TRUE COPY.

But these suggestions with the AttachmentAPI is good. Though the "has" function is not needed IMO, because usually has implementations are: "get() != null".
Edit: I hope that Attachments are copyable too.
Otherwise we come in with the "unrusted copy" problem again...

Also I think the CapabilityContext idea with getting things is a good idea, and using the custom Direction interface (BlockSide IIRC) is also a good approach.

Only one thing i am thinking about is how to deal with BE Capability Caches.
For BE maybe implement a general listeners you can attach to where you can say: I wanna listen to any "Capability invalidation" changes or something like that.

IC2C for example has fully customizable slot definitions where the user controls which slot is accessible from which side.
And if a side gets disabled i also notify other blocks that have hooked into listening to changes.

Maybe something like this:

public void onCapabilityChanged(CapabiltyType<?> type, BlockEntity source);

The name is just bad atm, it doesn't detect changes within the capability but when the caches that you build should be revalidated instead (like refetching of the capability)

I can see why LazyOptional has to get made rid of. Though a Listener system on a BE might be a suitable replacement.
Because that way blocks can notify each other of changes instead requesting data validation.

Honestly this is going towards a good direction from what i have read.

Makes me feel hopeful that Technic4n is not making a worse replacement.

#

Also if you are annoyed that i am constantly joining/leaving. Don't.
Its a waste of everyones energy xD

#

and dbza I T pop

quaint yoke
#

??? Alright then. Kinda hard for you to give useful feedback when you don't hang around to see how people reply...

#

A quick glance at the API looks pretty promising; I'll look over it more this weekend or sometime

#

Given the creative tabs issues it's not like there's really any alternative to always synced data on ItemStacks, and that part looks pretty good. Just to double check because I'm not quite sure I understand it - are we storing it in the ItemStack tag NBT or parallel to the other ItemStack data, with count/ID and whatnot?

raw hull
#

Feedback™️

Okay, so taking a look at item capabilities for an example, based on how I think I would use them (if I've made wrong assumptions about how this works, please correct):

  • I have a custom capability, so I create and register somehow (?) a ItemCapability. This is my singleton-query-thing.
  • RegisterCapabilitiesEvent is fired on startup, once (?)
  • for caps that I know what cap applies to what item, I would register those (i.e. a backpack-like item, currently exposed via initCapabilities, would use register(... Item)
  • for caps that currently rely on datapacks to select what items to apply to; I would have to call registerAll() with a single provider. But this provider is then doing the runtime check of "for this item stack, should it have this cap"
  • I want to store data (outside the stack tag, because I want serialization-time-persistence), so I need a attachment. so I MY_DATA = new AttachmentType(...), and I pass in MyData::new, along with nbt ⟷ MyData function.

I see myself then using the system something like this:

event.registerAll(MY_CAP, (stack, ctx) -> {
  var prototype = getFromDatapack(stack);
  return prototype != null 
    ? new MyImpl(prototype, stack, stack.getData(MY_DATA)
    : null
});

// where `getFromDatapack` will check essentially, a recipe defined in a datapack, and return some data useful to the impl (also, if it should exist at all)

And then later querying like so:

stack.getCapability(MY_CAP) // yes, but bounces to...
MY_CAP.getCapability(stack) // internal API, why?

Questions:

  • is this all correct / intended ?
  • is there no registry for attachments or did I miss something ?
  • can ICapabilityProvider be marked @FunctionalInterface ?
  • forge:caps instead of __FORGE_CAPS__ convention for serialization ?
  • why forbid (via APIStatus.INTERNAL) MY_CAP.getCapability() style query ?

That's it for now, just managed to fit in the max discord message with only 3 chars to spare

buoyant patio
#

does the cap registration happen before or after tags load?
it would be nice if we could register caps based on tags

severe axle
#

makes no sense to be post-tag

brazen verge
#

you can already determine attachment by tags

raven coral
# surreal frost well the issue with Sophisticated storages approach is that you can not really s...

oh yes I see the problem with sophisticasted backpacks... well that's an IItemHandler problem more than anything imo

right now get() will attach the default data if it's absent - hence the separate check

serializable attachments are copyable, yes

I think we'll stick with @Nullable Direction for block caps since nullable is not a problem anymore with the current design

for the cache I think it would just be a Supplier<@Nullable Capability>; this will be a lot faster than a full query, however you still won't receive notifications / invalidation; we might add an optional caching system but notifications will never be reliable if they are optional

hopefully you see this message soon enough xD

raven coral
raven coral
# raw hull **Feedback™️** Okay, so taking a look at item capabilities for an example, base...

™️

so I create and register somehow (?)
ItemCapability.create is enough to register it - kind of like a ResourceKey

RegisterCapabilitiesEvent is fired on startup, once (?)

  • yes the event fires once on startup

I see myself then using the system something like this:
are you the only implementation of MY_CAP? if so it should probably just be a helper method; otherwise yes registerAll is correct

public static MyCap getCap(ItemStack stack) {
  var prototype = getFromDatapack(stack);
  return prototype != null 
    ? new MyImpl(prototype, stack, stack.getData(MY_DATA))
    : null
}

is there no registry for attachments or did I miss something ?
there's a registry in ForgeRegistries - you can use a DR to register them

can ICapabilityProvider be marked @FunctionalInterface ?
yes; I forgot, thanks

forge:caps instead of __FORGE_CAPS__ convention for serialization ?
I expect this will be one of the last things we discuss. "caps" shouldn't be used anymore... maybe ForgeAttached or something like that (ForgeData is already used for the persistent tag in some places)

why forbid (via APIStatus.INTERNAL) MY_CAP.getCapability() style query ?

  • Orion said he preferred target.getCapability(CAP, context) so I just went with that 😛
  • we can provide an overload for Void context caps that doesn't require passing null if we query on the target object:
    having that said, for those caps that require context, I don't really see the benefit of stack.get(CAP, context) vs CAP.get(stack, context)
raven coral
#

ok, the PR now has a proper description

#

(see pins for the link)

raw hull
#

hmmmmmmm. interesting, interesting. you basically (for that use case) convinced me out of using capabilities at all (and I mean fair, I only need the data attachment part which right now implies a capability, so interesting mindset shift.

#

another question: does it make sense to, or is it possible to have a non-serialized field on an attachment? would there be issues you would run into?

currently, that would just be a field on the capability, but the capability objects aren't persisted under the current system so I'm trying to figure out where it would go.

examples:

  • an item acting like a furnace, in item form, which is caching the last used recipe for performance.
  • we have a non-serialized flag that marks an item as "magical" in the creative menu, that's meant to not be copy-able to the player inventory.
raven coral
#

yes if you don't provide a serializer to the data attachment

raw hull
#

otherwise I'm very much liking what I'm seeing here. (no lazy optional for item stacks, bless 🙏)

raven coral
#

for example AttachmentType.builder(() -> new Cache()).build();

raw hull
#

Data attachments don't exists on any sort of item stack construction, yeah? They're lazily initialized (from a default supplier), and then that instance persists on the stack?

what happens on ItemStack.copy ? The attachment gets serialized + deserialized again? I know Shadows had wanted to provide explicit .copy() implementations / alternatives, is that part of the plan or not worth doing for no real perf. urgency?

raven coral
#

explicit copy is planned, yes

#

but it's not a pressing issue because copying is only slow if the source stack has some kind of attachments

raw hull
#

fair enough

raven coral
#

and even then it scales based on the number of attachments and how much work it takes to serialize them

#

compared to firing an event that goes through ALL listeners that is not much

hardy breach
#

For item stacks I would still want the ability to attach an object to them because I store data that A: I use for rendering so need to access quickly (accessing NBT will be slower) and B: storing data in an item’s tag forces vanilla to sync it which I don’t want for cases where I have fast changing data that I want to sync using my own packets for performance reasons (vanilla resends the entire stack)

raven coral
#

Item stack data attachment is kept

#

Please see the PR

#

I just updated the first message to clear the misunderstanding for new readers 😉

#

Of note to you especially: serializable ItemStack attachments are always synced now as an NBT subtag

#

Not sure about syncable BE and entity attachments yet

hardy breach
#

Ahh my bad, didn’t see that. Would it be possible to split syncing and saving so that modders can choose what data is saved and what data is synced ?

raven coral
#

Not for item stacks, no. For BEs and entities yes

surreal frost
# raven coral oh yes I see the problem with sophisticasted backpacks... well that's an IItemHa...

The Sophisitcated Backpacks problem was actually with the FluidHandler in this case. Because Backpack Item interaction is rare, while fluid handler options are common....

Attachements:
If they copy with ItemStack.copy then that would fix Sophisticated Backpacks problem too... At least to some degree.

Direction:
That is fine. It is just a nicer implementation to use BlockSide because you can down to 1 function instead of two.

Caches:
The Optional Caching system sounds great.
You could make it more reliable if BlockEntities/Entities that extend the "CapabilityContainer" could just register listeners in the first place inside the TileEntity. Where TileEntity invalidation would trigger notifications, this would cover 99% of the cases and the 1% could be done by the modders themselves.
So instead of per Capability listeners you could do a Per BlockEntity (and maybe Per entity) listener.

And then the caches just use that system instead 🙂
Which is a way nicer approach.
Though i assume "unregisterListener" should also be a thing because memoryleaks xD

To be fair i wouldn't have seen this message any earlier then right now.
Also it gives me time to see how this argument turns out and give a levelheaded answer 🙂
Its better for everyone involved xD

surreal frost
#

Because that works the exact same xD

quaint yoke
#

And there's a reason this discussion is happening mostly here

surreal frost
#

i get why, the search is nice 🙂
But anyways more then a few messages you wouldn't get out of me per day anyways so nothing would change in me staying outside of me being more annoyed. And that is more problematic 🙂

#

I try to provide constructive criticism that helps avoid pitfalls i have seen done multiple times.

#

Anyways poof

radiant minnow
#

Still not a fan of caching when you can simply obtain a supplier and query it every-time

#

The problematic part is lookup, not access. So looking up the supplier and then using that forever looks better to me

raven coral
# surreal frost The Sophisitcated Backpacks problem was actually with the FluidHandler in this c...

ah hmmm, idk what is wrong with their fluid handler then except maybe forgetting to copy?

this would cover 99% of the cases and the 1% could be done by the modders themselves.
well that is the current system and it's been shown to not work, so I don't have any hope wrt. notification; for the cache I mainly want to cache the target block entity, but the actual capability query will need to be performed every time 🤔

radiant minnow
#

The supplier idea would work, as that would mean that the code path is faster: just have to essentially get the field representing the cap

#

But yeah, invalidation and friends simply don't work and aren't needed

raven coral
#

well yes the supplier should be very fast

radiant minnow
#

You'd need some sort of indirection in the API anyway, to avoid getting stuff that is invalid (think BE invalidation)

#

And then it would be up to the mods to implement stuff e.g. For sided caps

#

() -> exposesItemsOn(capturedSide)? sideHandler : null;

raven coral
#

yes that's the ICapabilityProvider

radiant minnow
#

Yeah, yeah

raven coral
#

the cache does the following things:

  • cache the BE
  • cache the ICapabilityProvider
radiant minnow
#

And the API would be sort of wrapping it like () -> be.isValid()? provider.get() : null; or something of that sort

#

Sure, I am avoiding all perf and object allocation concerns

#

Just trying to understand if that would be the general flow

raven coral
radiant minnow
#

Well yeah, but you don't have the supplier here lol

#

API API nets you the object directly

raven coral
#

well BlockApiCache is the supplier

radiant minnow
#

I'm more like, if you want to have a supplier<cap>

raven coral
#

well that would be the block api cache with a fixed context argument

radiant minnow
#

Could have that as a helper

buoyant patio
radiant minnow
#

That's just the callback

#

Like, what you already do

buoyant patio
#

that caches it (not looks it up every time)

raven coral
#

let's be clear that only the BE is cached

#

not the target object

radiant minnow
#

I'd argue that you'd only go through the API API cache to get stuff

#

Not yourself manually

raven coral
#

not sure what you mean - I guess we'll discuss the forge implementation when I'll have it written

radiant minnow
#

Essentially, you wouldn't query caps manually, you'd only call BlockApiCache.get

raven coral
#

ah; well the cache needs to be stored somewhere, so for many cases you'll be fine with a direct call

surreal frost
#

The block entity shouldn't be tested against at all when trying to read the capability.
Either the capability is there or not.

radiant minnow
#

If you use the cache, then you don't

#

From what I understand anyway: always call BlockApiCache.get(...) or whatever the new equivalent will be

surreal frost
#

Really so the cache doesn't in the background revalidate everything everytime you access the supplier?

#

In my current implementation the cache is a array access no hidden logic.

#

The callback that lazy optional currently provides simply clears the array entry when the callback is called.

surreal frost
radiant minnow
#

Useless, it's not properly implemented nor universal, so it doesn't make sense to have it

surreal frost
#

I ask you this if thousands of blockentities constantly check: is this valid? When that could be removed wouldn't there be a massive performance improvement?

surreal frost
raven coral
surreal frost
#

Define lazily?

#

On a as needed basis?
Because that would be at least once per tick check.

#

For a ton of cases.

#

And that assumes only 1 capability for 1 side is used. This amplifies with each capability.

#

A simple blockEntity invalidation callback would remove the need for so many custom implementations of it.

Most multiblocks have a callback included to simplify the process of the structure being deleted.

So we have multiple usecases for that.

public void onInvaldated(BlockEntity source, RemovalReason reason)

(Reason because kinda makes sense if custom ones want to be provided)

#

Pretty sure AE and other mods would benefit from that easily having a callback that notifies a blockentity being deleted.

raven coral
#

We really wouldn't

#

Since the actual IItemHandler operations are a lot more expensive than getting the IItemHandler in the first place

#

I think it should be the same for everyone

surreal frost
surreal frost
raven coral