Capability rework

[raven coral]

Yeah, probably? I'm not expecting people to override it in practice

There's already an attachment builder method for that actually

[buoyant patio]

yea that should probably be an actual class and not an anonymous one

[light oriole]

This is unfortunate but fair; I am right in assuming that you're talking purely about the "API access" part?

[raven coral]

yes

there isn't a ChunkType or anything like that which we could use for dispatch

if you want to expose a chunk-based API chances are you just need a static method in your API:

// API method
public static int getAmbientMana(LevelChunk chunk) {
    // Probably implemented using data attachments, but not relevant to API users :)
}

[light oriole]

Makes sense - I'll likely have to make a MyApi.for(thing) type- yeah

[raven coral]

usually you wouldn't share the same API between chunks, entities, etc

[light oriole]

in my case I am, since the API is for a temperature system where everything has a temperature

and the reason I'm using the same API is so that I can calculate temperature gradients and stuff regardless of what the "thing" is 😅

[rain tartan]

mmm, cleaner patches

[twin wedge]

Yay, that totally is the reason I haven’t gotten around to reviewing the PR yet (and probably won’t until tomorrow), yeah let’s go with that

[buoyant patio]

tech you broke all your patches

[raven coral]

yeah, oops - just pushed a fix 😄

[rain tartan]

y'know, I was wondering where the patch headers changes I was expecting were

(the number after the _)

[raven coral]

yeah ok that was too clean to be true 😄

[surreal frost]

Eh yeah @raven coral .
Your capability system has a vital flaw...
What if your BlockEntityType has a custom implementation that overrides the isValid implementation to always return true and has a empty set?

That automatically fails then.
Why I have such a implementation?
We actually require blockEntity types to be part of the block constructor...

Soo yeah...
It would be nice if there was also a global capability register option. Still bound to the what type.
But mainly because when you have hundreds of blockEntity with a common implementation but requiring hacks to test if accessible to reduce the amount of manual labor...

I am all for brute force... But one thing forge allowed is to reduce manual labor by providing base implementations that got all the grind work down to a minimum...

The new system doesn't allow that.

[severe axle]

That sounds like bad design on your part

[surreal frost]

Says the guy who uses overwrite everywhere because he is to lazy too make proper mixin hooks.

[severe axle]

Requiring the BE type be available in the block constructor violates the registry initialization order

[surreal frost]

Nope it doesnt

Otherwise it would crash like all other registries when you access them to early/to late. But it works just fine

[severe axle]

It might not crash outright but it still violates the established contract of Block->Item->Else

[surreal frost]

No. BlockEntities and blocks can be registered at the same time.

Because they are related.

Also that doesn't change the fact that we still would use a empty set + is true implementation on BlockEntityTypes since the checks are literally only used on rendering/tick.

(last time I checked)

I mean hear me out. What if you have a block entity that doesn't have tesr or update ticks? XD

[severe axle]

the checks are also used at placement and load to see if the BE is legal at that position

[surreal frost]

And what if a addon wants to recycle the blockEntityType?

Custom sign implementation that only changes the texture -> new blockEntity Type -> new BlockEntity renderer in the registry. Literally no changes in SRC besides Reregistering the same implementation.

[severe axle]

Generally you just make another BE type with the same underlying BE

Or you could modify the set of valid blocks, but it requires reflection/at

[surreal frost]

Or. Hear me out make a blank state blockEntity Type that just works for any block xD

Also depending on the loading order blockEntities could fuck with any capability with the current implementation and hide perfectly xD

[severe axle]

The reason you don't do that (and that the second option I listed is discouraged) is because BE's make assumptions about what the block will be when in-world

Doing either of those things can cause the BE to be on an invalid block

[surreal frost]

Question.
Outside of a mod intentionally outright doing it. But doesn't mc have a literal blockEntity cleanup system on blockstate change?

That the blockEntity controls?

Not the blockEntityType

[severe axle]

the Block controls the process for the most part, not the BE

[surreal frost]

Or the block

But isn't the ontick validation kinda just a performance drainer to ensure safe file correctness so nobody cheats by changing the safe file?

[severe axle]

The main issue is if you fail this check then you can have race conditions between the render workers as blocks change

[surreal frost]

Unless the blockentity gets removed in the first place by the block validation system...

Yes even on the client side

[severe axle]

Not how race conditons work, lol

The chunk render worker threads will pull the BE and the block during the change after the block changes but before the BE is removed

[surreal frost]

Yeah it might cause that a beacon beam stays 1 update longer then it is supposed to. But that is like barely and issue xD

[severe axle]

It will crash if the BER accesses a state property

[surreal frost]

Huh...

Ok I have a case for that actually xD

Good to know. XD
Though that applies only 1% of all blockEntities.

Maybe even less depending on your mods installed

And it is technically only client side?

Yeah for Capabilities not a good idea unless brute force is used xD

Anyways @raven coral I still highly suggest that there should be a global registration system because if shared implementations are used (abstract classes) and you have hundreds of implementations (extending the abstract classes) that can not be shared it's going to be a bad grind...
Would be nice if that could be simplified somehow?
Otherwise a solution would be: register all capabilies that neo has and check during runtime if it is valid. And let's apply every mod block into the blockEntityType list to make sure it always works xD
(Defeats the point ofc xD)

( I am fairly simple to please. Just don't make me do grind or I find a way to hack my way through it)

[mint current]

It's not a per-tick check, it's checked on blockstate change. If it were a per-tick thing, then performance would go down the shitter really quick

[surreal frost]

That is done on the Block or BlockEntity side not the BlockEntityType.
Because you technically make it conditional based on the state while the blockEntityType only compares blocks.

[dull ledge]

What do you mean by "grind" here? If you want to register a cap for every BE type in your mod, can't you just loop over every registered type?

[raven coral]

this only matters if you are registering your capability with a BlockEntityType in the event; if you register with a Block instead then you don't have to worry about isValid and all of that

    /**
     * Register a capability provider for a block entity type.
     * <p>
     * <b>If a previously returned capability is not valid anymore, or if a new capability is available,
     * {@link Level#invalidateCapabilities(BlockPos)} MUST be called to notify the caches.</b>
     * See {@link IBlockCapabilityProvider} for details.
     */
    public <T, C, BE extends BlockEntity> void registerBlockEntity(BlockCapability<T, C> capability, BlockEntityType<BE> blockEntityType, ICapabilityProvider<? super BE, C, T> provider) {
        Objects.requireNonNull(provider);

        IBlockCapabilityProvider<T, C> adaptedProvider = (level, pos, state, blockEntity, context) -> {
            // TODO: not sure that the != type check is really needed; but just in case?
            if (blockEntity == null || blockEntity.getType() != blockEntityType)
                return null;
            return provider.getCapability((BE) blockEntity, context);
        };

        for (Block block : blockEntityType.getValidBlocks()) {
            Objects.requireNonNull(block);
            capability.providers.computeIfAbsent(block, b -> new ArrayList<>()).add(adaptedProvider);
        }
    }

you can just call registerBlock for all your blocks instead with the adaptedProvider 🙂

[surreal frost]

So I just register every ic2 block with a item/fluid/RF capability provider in neoforge and just say if a block doesn't have a capability it returns null?
Even if a lot of blocks don't have blockEntities... (We have a lot of blocks with and without blockEntities)

[buoyant patio]

you only register it for the blocks that can have that capability and return null if they don't

[surreal frost]

how would you automatically filter that if you have hundreds of blocks?

without manually defining it for each block...

[buoyant patio]

you know which do and not do right? so just do it like creative tabs and add them individually or via a list

[surreal frost]

I can either filter by blockEntityType or by Block, each implementation "could" have it, but without instance checking i can't tell you 🙂

and i mean blockEntityClass instance checking.

[raven coral]

worst case you just register the capability for every ic2 block

[buoyant patio]

again if a block can have a capability you register it, that does not mean it has to have it

if it does not at runtime you just return a null capability

[surreal frost]

public static void registerBlock(Block block) {
    BlockEntity entity = block.createBlockEntity(block.defaultState(), BlockPos.EMPTY);
    for(Capability<?> cap : allCapabilitites) {
        if(entity.hasCapability(cap) != null) {
            //Register this capability?
        }
    }
}

(I know there is an event, but this would be my solution so far xD)

[raven coral]

yeah that's perfectly fine

[surreal frost]

again, i am not manually writing out by hand each implementation that has it xD

[buoyant patio]

there technically is no more thing ad BE capability only Block capability

[buoyant patio]

creating a BE during registration is everything but fine

[surreal frost]

well either that, or a dynamic support implementation xD

[buoyant patio]

if you want to use your special system you have to

the BE capability stuff is just a helper for Block capabilities

[raven coral]

I'd say it's fine to create it in the cap event

[buoyant patio]

to support your niche usecase we'd need to deoptimize the whole system

[surreal frost]

not really, as a fallback xD

[raven coral]

you can already implement that fallback in the current state of the PR

you can perform the instanceof check inside the provider, for example

[buoyant patio]

but speiger does not want to write out the blocks that can have the capability

[surreal frost]

technically only because BlockEntityCapablities get mapped to blockCapabilities xD

[buoyant patio]

but there are no more BlockEntityCapablities

it is just a helper

[surreal frost]

But @raven coral (Sorry wish discord had name picker like on mobil where you could remove the ping)
Is right as long as i could register a Dynamic BlockEntityProvider on ever IC2 Block then i wouldn't care.

which is possible atm.

Lets put it this way, i would do it the exact same reason why IC2C has either 0 json files or each json file is 100% auto generated...
And i mean very little backing code behind the json file.
(we have roughly 50-100 json files in total)

[buoyant patio]

you can write @silent at the beginning of the message to suppress the ping

[surreal frost]

Here yes, but not if i dont want to manually type out Schuri, because i will typo it 100% chance (this time to make a point)

especially with a complicated name like technic4ian

on discord mobil i can just click on his name, remove the @ and i have a perfectly typed name.

[buoyant patio]

if I type an @ it gives me the list of people

[surreal frost]

Yeah now try to remove the ping out of that name on discord PC version.

it will remove the entire name xD

[buoyant patio]

just write @silent at the start of the message and don't bother about the rest

[quaint yoke]

I'm not sure I see the issue here. Can't you just attach your capability to every block that needs it? What prevents that?

[surreal frost]

@raven coral i would write @te then hit enter on tech for example and then i cant remove the @ or modify it xD

sry again xD

[buoyant patio]

they don't want to list out stuff manually

[surreal frost]

Until now i wasn't sure if that was a bad idea to begin with since i haven't looked to deep into it since it was 4AM when i wrote that xD

[raven coral]

you just do

event.registerBlock(ITEM_HANDLER, block, (level, pos, be, state, side) -> {
    return be instanceof Ic2ItemHandlerProvider handlerProvider ? handlerProvider.getHandler(side) : null;
});

[surreal frost]

nah i wouldn't do that. Thats still not enough xD

[quaint yoke]

Yeah I'm still not seeing the issue. Just add the blocks to a list, add the cap to the blocks

[buoyant patio]

they don't want to do the list part

[quaint yoke]

But... why not? Just "because I don't want to"?

I don't think that's something to worry about then

[surreal frost]

public void registerBlockCapabilities(CapabilityRegisterEvent event) {
    for(Capability cap : allCapabiltities) {
        event.registerBlock(cap, block, (level, pos, be, state, side) -> {
    return be instanceof ICapabilityMaker handlerProvider ? handlerProvider.getCapability(cap, side) : null;
});
    }
}

[surreal frost]

write hundreds of selectors manually. Love the bug reports: "WHY ISN'T MY MACHINE ACCEPTING ITEMS" oh yeah forgot to register it.

[buoyant patio]

why all capabilities?

[quaint yoke]

Who said anything about writing them manually...

[surreal frost]

because maybe i am adding support for it?

(allCapabilities => allCapabilities for blockEntities onlyOfc)

[surreal frost]

that includes the lists for everything.

[quaint yoke]

Just have a helper that your block construction is wrapped in, that checks if the block should have the capability attached and adds it to a list if it should...?

[buoyant patio]

so if my mod adds a random Block cap your block will have it?

or do you mean all default caps

[surreal frost]

we have roughly 450 block instances in IC2C.
A LOT of them need different capabilities.
Would you write that manually by hand for me please?

[quaint yoke]

No, of course not. But you know which blocks you're registering. You can easily, when a block is constructed, inspect the block to see whether it needs caps added, and add it to lists if so. Programatically... so nothing is by hand...

Like, I'm not seeing an issue here

[buoyant patio]

can't you just iterate over your DR and then instanceof the blocks?

[surreal frost]

we have roughly 20-50 block classes due to shared implementations.

[quaint yoke]

So?

Not seeing the issue

Heck, you want to do this based on block entity type?

You can probably grab that from the block if you'd like!

There's a million different solutions here

[buoyant patio]

you can use marker interfaces (or even ones that have functionality) on the blocks

[quaint yoke]

None of which involve manually listing everything out, if that's your worry

[surreal frost]

would not work since for the same reason our BlockEntityType just returns true in the "IsValid" and has a EmptySet xD

because i am not mapping that out for like 5 blocks that need it for blockEntityRenderers drawCalls.

[quaint yoke]

So? How do you currently add capabilities? It's based on the BE, right?

[surreal frost]

we have a abstract class that is called: BaseInventoryBlockEntity that auto adds it, for example.

[quaint yoke]

So go through your blocks, find the ones with a capability-capable BE type, and expose the relevant stuff.

[surreal frost]

that is like the second or third layer of all implementations.

[buoyant patio]

well have that same thing but as an interface on the block and use that

[quaint yoke]

Or heck, shove an interface on the block classes which should expose the capability... like, this is not a hard problem.

There's a million different ways to solve it. Yes, you'll have to redesign some stuff

[surreal frost]

multiblocks wouldn't work because it has them disabled until the machine is formed valid... Technically i could make a "ghost" reference that says it has it. But that would force me still to either register all or create a blockEntity during creation to validate it xD

[quaint yoke]

That's called adapting to change

[buoyant patio]

or even better use BEType tags

[quaint yoke]

Just register the capability provider and have it return null when it can't make one, right?

[buoyant patio]

because that's why they exist

[surreal frost]

nah my potential solutions is adapating to change. Because brute force is requested.

[surreal frost]

manually hand writing that i love to do so much xD

heck we even auto generate our tags xD

[quaint yoke]

There is no reason you have to use a brute force solution

[buoyant patio]

that is what datagen is for

[surreal frost]

no the default implementation requires brute force xD

[quaint yoke]

Just rethink your design so that a programmatic solution works well

[quaint yoke]

No...?

It literally doesn't

[surreal frost]

really it doesn't require me to manually define each capability to block/blockEntity?

[quaint yoke]

You just attach stuff to blocks. How you do that, brute force or programmatic, is entirely up to you

[surreal frost]

show me where?

programmatically => register a dynamic Capability provider that test during request.

[quaint yoke]

No, programatically => filtering your own stuff to programmatically form a list of what needs capabilities attached to it

That's the paradigm now. Stuff gets attached to flyweights

[surreal frost]

how would you filter that if you have only like 20 classes that share like over a 100 implementations?

false negatives are just to likely to happen

[quaint yoke]

I... would filter them? Like, I just would?

I'd make my implementations and classes expose methods that let me figure out what needs attached to it...

And then I'd query those methods to figure out what I need to attach

And then I'd attach it...

[surreal frost]

literally the only choice is:
if(block instance of IC2SharedBlockEntityBlock) //Apply capability and that classname doesn't exist just to make a point.

[buoyant patio]

public void registerBlockCapabilities(CapabilityRegisterEvent event) {
    for(Block block : BLOCKS) {
        if (block can have cap) {
            event.registerBlock(cap, block, (level, pos, be, state, side) -> {
                return be instanceof ICapabilityMaker handlerProvider ? handlerProvider.getCapability(cap, side) : null;
            });
        }
    }
}

[quaint yoke]

...no, why is that the only choice? You can make IC2SharedBlockEntityBlock expose methods that tell you what sort of capabilities to expose...

You can add methods to your classes that expose the necessary information

[surreal frost]

that is what i suggest xD

[quaint yoke]

You don't have to go based on only class

[surreal frost]

and he says NO DON'T DO THAT

[quaint yoke]

You can also just expose every capability that way, sure. The point is that you have tons of options that work just fine

[surreal frost]

but literally t hat is registering a dynamic BlockEntity Provider

[quaint yoke]

There's no "you're forced to do it that way". The only thing you're forced to do is attach capability providers to flyweights because that's how stuff works

[quaint yoke]

No. Because it's attached to the block

[surreal frost]

you just want to make me cry xD

[buoyant patio]

the important bit is that you only attach to blocks that actually can have the capability and not just all

[rustic crystal]

Honestly terrible system we should just go back to interfaces (not sarcasm)

[surreal frost]

Yeah but that filter is literally simple to write, Everything before that sounded like: Write a filter that detects specific capability types instead of all of them.

[quaint yoke]

Doesn't work well for attachment to flyweights in practice, painful to cache so it gets really slow, and leads to jankiness attaching stuff to vanilla stuff

Also, composition before inheritance

Optionally attaching interfaces for mods you have a soft dependency on is painful if you use inheritance

[surreal frost]

I know what you are saying, but even with that applied that wouldn't work xD You can make composition detection if the composition is dynamic xD

[buoyant patio]

well you should check if the block can have that capability and not just plaster all caps to all blocks

[quaint yoke]

You... kinda can though. The capability provider can return null. All you need to know is that that block might hold a capability of that type...

[rustic crystal]

Cache i get, but it provides a level of flexibility thats hard to get otherwise, plus id much rather implement than register since one is 10% of the code of the other

[surreal frost]

yeah and that is where writing hundreds of blockInstance (not classes) lists would be required...

and that is what i want to avoid..

[quaint yoke]

I... still don't see how that's necessary

[surreal frost]

with entires, not the amount of lists themselves.

[quaint yoke]

Move the logic around so that you're thinking in terms of the block, not the block entity

[surreal frost]

yes

[quaint yoke]

That doesn't require giant lists of block instances...

[buoyant patio]

just make a helper that adds them to a list when registering, alternatively use an interface on the block to indicate the capabilities it can have

[surreal frost]

technically our blocks are composition based, you dont create unique instances anymore but you create something like this: IC2Blocks.with(DROP_HANDLER).with(BLOCK_ENTITY_TYPE).with(TEXTURE_PROVIDER).build();

[buoyant patio]

well then add with(capability)

[surreal frost]

technically that would work, but if it can be avoided I will not do that.

[buoyant patio]

and have a multimap from capability to block for registration

[quaint yoke]

How do you currently tell which capabilities to expose?

Like, when actually exposing a capability, what do you check?

[surreal frost]

ok lets put it this way: I am trying to reduce the amount of potential "Oh i forgot to do that" bugs.

[rustic crystal]

This reminds of javascript absolute pain xD

[quaint yoke]

I assume you check some data in the block entity. That data has to come from somewhere. Follow it back enough and it's eventually coming from something related to the block

(or the block entity type)

You can just shift that all earlier so that the relevant stuff is on the block

[surreal frost]

Ignoring the dynamic ones, (if we go really specific technically all of them)
Yeah i will go with the dynamic provider because that is less code then manually typing which capability goes to which block.

[quaint yoke]

You still don't have to do that in all likelihood

[surreal frost]

maybe less efficient but since capability caching is a thing i don't worry to much xD

[quaint yoke]

Speiger, how do you currently tell which capabilities to expose?

Like, when you're actually asked for a capability

[surreal frost]

I have a map.
And upon the constructor of each abstract implementation i simply add it to the map.

[quaint yoke]

Right. Each abstract implementation being an implementation of what? The block? The block entity?

[surreal frost]

for some i do it manually but that is very little. Like 20 implementations in total maybe?

[surreal frost]

since forges is blockEntity based, blockEntity ofc xD

[quaint yoke]

So you have a map of block entity creators to capabilities they expose somewhere, right?

Unless you mean that's stored as a map within each block entity. In which case, how do you populate that map?

How do you know what to fill it with

[surreal frost]

Map<Capability, LazyOptional> capabilityAccessors;
public void addCapability(Capability cap, CapabilityInstance in) {
    //Insert lazy optional
}

public LazyOptional getCap(Capability cap, Direction side) {
    return capabilityAccessors.getOrDefault(cap, CapabilityInstance.empty());
}

and each abstract class calls addCapability

generics instead of CapabilityInstance ofc

[quaint yoke]

Each abstract BlockEntity class?

[surreal frost]

InventoryBlockEntity adds the inventory instance. FluidBlockEntity (technically extending InventoryBlockEntity) adds the fluid Instance etc etc.

[quaint yoke]

So somewhere, you've got a bunch of block entity classes. Each of those has logic in it to add capabilities to a map. And each block has logic in it to create a block entity of the correct type, do I understand that all right?

[surreal frost]

i am awaiting your point.

technically yes, depending on how you want to view it.
What you suggest is like a integer wrapper.

[quaint yoke]

I want to make sure I understand your system first. That is how it works?

[surreal frost]

so if it is a integer wrapper then yes.

[quaint yoke]

Your blocks are aware of what sort of BE they make?

[surreal frost]

No they are not

[quaint yoke]

Then how does a block create a BE?

[surreal frost]

It is literally: BlockEntityType<BlockEntity> instance. It doesn't know the Type of BlockEntity it creates, it just knows it creates "A" BlockEntity.

and this is why i was very cryptic because i was awaiting your point.

OH I get your point... Make a dynamic BlockEntityType.

Thats actually a great idea xD

Though that wouldn't work well either...

but it would solve the "isValid" problem...

[quaint yoke]

I hadn't actually made any point yet, I was collecting information because you sorta have to do that first

Can't you just have blocks hold a block entity type alongside an object that holds logic for which capabilities that type holds and how to get them out of a block entity of that type? And construct such an object alongside your BlockEntityTypes?

[surreal frost]

you are again at the writing that out manually part?
For over 200 blockEntityTypes/blockInstances. No

[quaint yoke]

No, you'd generate it

[surreal frost]

how would i generate it?

[buoyant patio]

you said you have those with methods

[quaint yoke]

You already have that logic in the blocks, to call addCapability. Move that logic earlier...

[surreal frost]

again that is blockEntity

[quaint yoke]

Sure. Move that logic earlier

[buoyant patio]

why not with(capability, way to get it)

[surreal frost]

not writing that 200 items+

[light oriole]

i'm only just tuning in, but it sounds to me like you could just have an enum and then register based on the enum...? i do that with a bunch of my items

[buoyant patio]

but you already did for all the other with methods so where is the problem?

[quaint yoke]

Like. Instead of "block entity class ctor has addCapability calls, registration makes a block entity type", you'd have "block entity class has static method to make a paired block entity type with a capability exposer". Same amount of code in the end

[surreal frost]

my current system makes me write that once or twice in total. Maybe like a few times more for very specific one offs, but that is it.
We are talking about 25-50x increase for defining things.

<10x i work with, above that OH GOD no xD

[quaint yoke]

Still not seeing the issue with storing a paired exposer and BE type, and creating that through a static helper per block entity class so you have the same exact amount of code...

[hallow ledge]

is it 200 different caps? If not, just make a helper?

[surreal frost]

Block instances * Capabilities used.
Yes that can be midigated to BlockInstances * 1 but its still Blockinstances * 1, while atm i have < 15 to type manually.

i have over 200 blocks with blockEntities.

[radiant minnow]

The reality is that you are writing stuff manually anyway

Just in a different place

[surreal frost]

autogenerator it is xD

[radiant minnow]

Your BE has to provide the capability

Therefore you are already providing it manually somehow

[surreal frost]

with a dynamic BlockEntity capability fetcher

[quaint yoke]

That's been my point, yes

[surreal frost]

which was already suggested xD

but lukebemish thinks thats a bad idea.

[quaint yoke]

So move that dynamic ness to the block entity type level, by creating a pair of a block entity type and some capability info... and call it a day

[quaint yoke]

No, I've laid out exactly how to do that sort of dynamic ness correctly at the block entity type level...

[buoyant patio]

if you would show us your current code (as in all of it) then we could help your specific case

[quaint yoke]

In a way that's exactly the same amount of code

[surreal frost]

if the block was doing the logic, i am all for it. But the blockEntity is doing the heavy lifting. Especially with dynamic enabling/disabling capabilities that is hard to do on a block. Near impossible without referecing the BlockEntity in some way. Lets move it to blockEntity since ti needs it anyways xD

not saying neos implemnetation just mine.

[quaint yoke]

If block entities have to expose different capabilities, they should be different block entity types

*generally speaking

[light oriole]

yeah this would be my suggestion too, some sort of enum BlockEntityCapabilities { First(be.class, capReg, ...), ...; }

[buoyant patio]

but giving us partial information and it always boiling down to I don't want to write stuff out is getting tiresome

[quaint yoke]

Oh sure - I wasn't even using an enum. I'd just have a static helper per BE class that sets up the BE capability exposer and block entity type and all of that

[surreal frost]

i mean we are actively writing the amount of text required by me to write by hand. So i have to share the pain of that process to make you understand xD

[light oriole]

or even some sort of static map using clinit would work I suppose; an enum just means you have very predictable classloading

[quaint yoke]

Speiger, there's a million ways to do this without writing tons of stuff by hand. We've shared several. You keep boiling it down to "fully dynamic or tons of handwritten stuff" but that's just not true

[buoyant patio]

you know that you have written in this channel more than you would have if you had just written it out by hand

[quaint yoke]

Oh yeah, I wouldn't use clinit. I'd do it during registration

With a helper so you're not doing it manually to register each thing

[light oriole]

yeah, sometimes the amount of time saved by trying to be smart isn't worth it and just writing everything out even by hand is the easiest approach

[surreal frost]

I mean at this point i have already made up my mind with @buoyant patio solution, which is a great one at that.
I am just trying to make @quaint yoke understand that i am not writing that many lines of code for it, If he wants to he can xD (not in my implementation, due to Me being forced not /not wanting to open source ic2c ofc)

[quaint yoke]

Like, ```java
class MyBlockEntity extends BlockEntity
public static Pair<CapabilityExposer, BlockEntityType> create(whatever) {
...
}
}

[quaint yoke]

That's where you're wrong. My whole point has been that you can do this in a sensible way without writing that many lines of code

[surreal frost]

the solution that @buoyant patio makes is 50 lines at most. You are suggesting a +300 line of code suggestion xD

and the one you suggest right now is above 2k lines.

[quaint yoke]

WTF no my suggestion is not that many lines of code

[buoyant patio]

ok speiger, luke can I have this channel back for my review of the PR

[surreal frost]

each block i have to manually define the Capability exposer to is 1 line.

[surreal frost]

tell lukbemish to shut up and we are done. He is keeping it alive.

[quaint yoke]

Schurli, it's all yours. I have no interest in helping someone who doesn't want to be helped

[buoyant patio]

1. I can't I'm just a maintainer not a moderator
2. you 2 can continue in #modder-1․20․1-support

[light oriole]

yeah, it's time for you both to stop since you're both keeping it alive 😅

review away schurli

[surreal frost]

also to one answer your suggestion and then i am done.
No, bad idea to use enums on something that may be extended by addons. Which is effectively trying to map randomness to an enum.

[rain tartan]

who's going to write the migration primer to the new attachments and capabilities system

[surreal frost]

anyways, thanks @buoyant patio for your suggestion. Since i wouldn't have applied that basic filter without your suggestion.

anyways poof

[buoyant patio]

we possibly could face another big change for block capabilities in the future in changing it from instance based to holder based (because of the json blocks stuff mojang is making)

[light oriole]

I'm too lazy to make it but we totally need one of those "all the things!" memes for holders

[dull ledge]

I'd expect BlockEntityType's handling of valid blocks to also look a bit different once Mojang have finished with this, so that may guide how caps deal with it.

[raven coral]

that's just changing a few places, most of the code is Block-agnostic

[buoyant patio]

shouldn't CapabilityListenerHolder line 62 be a continue instead of a return?

[raven coral]

yeah it should be

[buoyant patio]

why is EntityCapability not a record?

[raven coral]

because we don't want people to instantiate it

any instantiation has to go through the registry which will perform suitable ID checks

[buoyant patio]

record constructors

[quaint yoke]

I assume in that scenario it would still end up being instance based as you'd want to re-collect capability providers whenever the registries get reconstructed, but yeah, fair point

[raven coral]

would be interesting to see how mojang would manage the lifecycle of all these block-based maps

[quaint yoke]

I'll be curious to see. I suspect we'll slowly see it all getting datapacked in some form

Heck, maybe we'll get built in registry attachment

Though there's a lot of other approaches they could take too

[buoyant patio]

Aaaaannndd review done

[twin wedge]

haven't looked at the code yet (so these comments might be somewhat moot) but am going through the PR description and under Usage for:

Some objects need to be marked as dirty explicitly:
I assume that is some objects (say chunk vs player) need it marked explicitly and not just some attachmenttypes? Also if there isn't documentation in the code about which ones need to be marked dirty explicitly there probably should be

[raven coral]

that was a bit outdated. I just updated the description, it should be clearer now

[twin wedge]

one comment on the way to create caps now is it does make it slightly harder to ensure they don't cause any class loading shenanigans. For example this: https://github.com/mekanism/Mekanism/blob/1.20.x/src/main/java/mekanism/common/integration/energy/fluxnetworks/FNEnergyCompat.java#L25 I will need to move attempted creation of that off into another class to ensure the param I pass to create (to get the lookup) doesn't crash when that other mod isn't present.

And as a side note does this mean that there are separate "capabilities" for say energy on blocks vs energy on items? So basically there will need to be chained lookups on piece of code that in the past could just take a capability provider? I am guessing at least once the capability is looked up it should be able to be merged back into a single line though right?

[quaint yoke]

How you look up capabilities differs between items and blocks and whatnot, yeah

Because there's completely different approaches you should be taking to doing that anyways

Like, looking it up for a block can be (and should be) cached in ways an item can't be

But the actual looked up capability should be the same, yeah

[twin wedge]

your example link in the description to the composter is a 404

actually seems most of your links 404

so given I can't easily look via that, does that mean you now have to use the event to attach caps to your own content?? Or are there still methods on the things to be able to expose them?

other than those comments the description seems reasonable enough at least until I get onto looking at the code and potentially the impls of some things might have some issues I don't know yet

[raven coral]

First, thanks for having a look

I will need to move attempted creation of that off into another class to ensure the param I pass to create (to get the lookup) doesn't crash when that other mod isn't present.
yeah that will have to be moved to a separate class - the flip side is that this shouldn't require much effort and we don't need any of the ASM hacks anymore 🙂

And as a side note does this mean that there are separate "capabilities" for say energy on blocks vs energy on items? So basically there will need to be chained lookups on piece of code that in the past could just take a capability provider? I am guessing at least once the capability is looked up it should be able to be merged back into a single line though right?
yeah the caps are separate: https://github.com/Technici4n/NeoForge/blob/rework-caps/src/main/java/net/neoforged/neoforge/capabilities/Capabilities.java

[raven coral]

I think it's the package rename, I'll fix that in a few minutes

[raven coral]

everything will have to go through the event now, that's correct

[raven coral]

updated all PR links as well, was indeed a package change problem

[twin wedge]

I assume this also means we are back to capabilities being able to be null? Rather than always being nonnull and then having a isRegistered method on it to see if the mod adding it actually did?

[raven coral]

If you call get on a capability you register it

So you can't check if a capability exists anymore because you will just create it instead

[twin wedge]

tech when you get the chance could you rebase your fork/merge upstream into it? When I start poking the code in the next few days I am going to poke at it with via mavenlocal and prodding it by updating a local branch of Mekanism to use it. So I can better see if there are any functionality gaps in the system. (And I need the latest NG for mek to compile which means I will also need https://github.com/neoforged/NeoForge/commit/10c294afb9ff19eb8218702beecc266891564fd6)

[raven coral]

sure

[raven coral]

done

[twin wedge]

Thanks

[twin wedge]

this is going to be such a pain given how I have things structured... so I will need to figure out what parts just need restructuring and which parts (if any) are no longer possible.

Question though, is the reason that you can only invalidate ALL caps at a given position rather than a specific one just to reduce the overhead of checking the type matches and that the theory is the overhead of all things listening to and having to recheck whatever mods are caching it are negligible?

[raven coral]

Yeah I think the overhead of invalidating everything is minimal

And invalidating all caps makes invalidation a lot easier in general

[twin wedge]

sounds reasonable enough, especially given ideally things shouldn't be constantly invalidating their caps anyway

[twin wedge]

under the registering capabilities part of the description it says there are two kinds:

1. register
2. registerAll (which functions as a fallback)
   Either I am blind/crazy or there is no registerAll in the RegisterCapabilitiesEvent, though at a glance it looks like it basically can just have multiple per type (which seems reasonable, and for some things definitely a bit better than registering to everything). Am I understanding that correctly?

And assuming I am then I have two comments (and I can also make them on the PR):

1. imo similar to the old system the object itself (or the mod adding it really) should have the first choice at providing an implementation for the capability. Is the intended solution that people who are attaching to their own objects and also other things just have to listeners and then set them at different priorities so that they end up earlier in the list of providers?
2. Given it seems like there is no registerAll, is there a clean way to say register that a capability can be on all living entities? And if not we probably want to provide some sort of helper method to do things like that so that each modder doesn't have to implement some sort of looping all entity types, figuring out if they are potentially living, etc

[raven coral]

yeah you are right... registerAll was removed in favor of just calling register on every single registry object if it really came down to that

imo similar to the old system the object itself (or the mod adding it really) should have the first choice at providing an implementation for the capability. Is the intended solution that people who are attaching to their own objects and also other things just have to listeners and then set them at different priorities so that they end up earlier in the list of providers?
IMO you should just register most stuff at the normal priority unless you have a good reason not to - this only matters if multiple mods are trying to provide a capability to the same object, which should be a rare occurence anyway?

Given it seems like there is no registerAll, is there a clean way to say register that a capability can be on all living entities? And if not we probably want to provide some sort of helper method to do things like that so that each modder doesn't have to implement some sort of looping all entity types, figuring out if they are potentially living, etc
I think forge registers an item handler capability to every single entity that checks for instanceof LivingEntity

[twin wedge]

I see... well that is a bit of a meh way of doing it but I guess. What we do is register it for ALL entity types and then in the provider itself check if it is a living entity, rather than say making it so only types for living entities even have the chance of exposing it

though I guess there isn't really a better way of doing it due to type erasure

[dull ledge]

Ahh, I had missed that registerAll was gone, than you for pointing that out Sara. When you say priority here, is this on the register event?

[twin wedge]

when I say priority I mean the event listener priority yes

because the providers it tries against is backed by a list so basically listeners that get fired earlier and then add for a type will be checked against first

[cerulean kernel]

I think forge registers an item handler capability to every single entity that checks for instanceof LivingEntity
ugh

(at the same time, regardless of my feelings on the livingentity item handler, registering based on entity class is a valid use case, and we should arguably have some system to not only support it, but support prioritizing by type specificity)

how much would break if at entity type registration we constructed an entity with a dummy level to extract its runtime class, given that most of the real work in most entity classes happens in the load function anyway

[twin wedge]

yeah that is sort of my point here ^. That yes it is done by NeoForge for some cases... but the fact it adds to all regardless of if they are living is a bit meh

[twin wedge]

or even just in addition to EntityCapability.providers have a list that lazily populates/adds to that map. Which yes would have a bit of a performance hit, but if done well it would only have to be looped the first time a specific entity type's caps are queried. And then given it adds to the providers the entries it could be skipped for each future call to getCapabilities for that entity

[buoyant patio]

that isn't 100% secure because an entity type could technically instantiate different instance types

how did forge populate the living entity list in the old capabilities event?

[cerulean kernel]

technically, yes, but with only the level and entity type itself as inputs there's not much they can do practically. maybe a separate client and server side type.

[cerulean kernel]

capability attachment was to an existing entity rather than a type, wasn't it?

[we also have the entity class problem in an unrelated spot, model layers, though there what people generally really want to do is find all entities whose renderer is of a particular type, but the event assumes that the renderer will be a LivingEntityRenderer and crashes otherwise]

[raven coral]

Forge used to override getCapability in LivingEntity

[raven coral]

priority gets a bit trickier I suppose if we add class-based entity capability queries

because you won't be able to mix and match the order wrt. entitytype-based caps

[buoyant patio]

ah it was the EntityAttributeModificationEvent that had the list of living entity types

[quaint yoke]

So, interesting question that popped up with this discussion in my mind: when does the capability event fire? And how plausible is it to make the answer to that "after data reload"?

[raven coral]

It fires before common setup atm

[twin wedge]

to be fair though, realistically class based could just be effectively lazily added to the types and I don't know if it really matters if it is lower in the order because in general if you want something say attached to all living entities then you are fine with it being more of a "fallback" anyway

[quaint yoke]

Personally I'm against class based attachment

I'd say, instead, that the cap attachment should happen every data reload, and then you could use tags for stuff like that

Though if the thing in question relies on the implementation of LivingEntity I suppose I see the point... I don't like the idea of doing stuff like that off of the constructed object, instead of the flyweight object, though

[twin wedge]

well actually... given the way the providers are kept track of is:
capability -> type -> provider
I guess maybe it doesn't really matter that much (especially in cases where you may be able to reuse the lambda) as while yes you are attaching it to entities that realistically shouldn't have it... in general you probably won't ever be querying your capability on them

[raven coral]

My thought was that it shouldn't be too bad to attach to too many entities because that will only cause a very slight slowdown for that particular cap

And we can add class-based lookup in the future quite easily

[raven coral]

I don't think it needs to be reloadable

[quaint yoke]

I think it should be. People may want differing capability attachment based on data. Additionally, it future-proofs the system substantially, though that's not as big of an argument in my mind

But that's always something that can be added when it becomes more of an issue

[raven coral]

You can put a data-dependent check in the cap provider

[twin wedge]

yeah that is partially what I am realizing that odds are it may not actually matter that much

[twin wedge]

hmm idea: overloads that instead of just vararg also accept iterable? That way at the very least you can say pass in things like BuiltInRegistries.ENTITY_TYPE and only allocate a single lambda for all the impls rather than having to handle it manually (for example CapabilityHooks is currently making a new lambda for each entity type

[raven coral]

Yeah why not

[raven coral]

well actually, the lambda in CapabilityHooks is only allocated once 😛

and I'm not sure doubling the number of methods in the event is worth it

[twin wedge]

There we go... made a decent bit of progress towards being able to better understand the system in order to review it and more importantly be able to test the PR

currently at about 250 changed files in mek due to it though ;-;

and this is pre cleanup and fixes and stuff

[raven coral]

😅

[rain tartan]

note to self: improve mods list screen someday

[twin wedge]

Well at least some of the builtin capabilities seem to be working

[rain tartan]

composter

[twin wedge]

yep something mek hasn't supported because of it not having caps, and with the rework blocks can have caps not just BEs so I was using the composter to test a bit of my logic to make sure I convert it over properly

[rain tartan]

👏

[twin wedge]

but yes finally after a few days of lots of work, I am getting to the point I can start testing the capability PR, and have a better idea how the system works for when I read through the code of it and leave comments

[twin wedge]

well I will need to look into that more tomorrow... but I don't think capabilities are currently invalidated on block removal

[twin wedge]

I still have a bunch to review, but went ahead and posted the comments I have so far just in case you decide you want to start prodding them

[raven coral]

Thanks a lot, I'll have a look 🙂

[twin wedge]

there left some more of my comments. Still have 10 files not marked as "viewed" that I need to go through tomorrow (some of which I already left comments on parts of, so odds are there won't be that many more)

als afterwards and after the comments are addressed (as some relate to javadocs/adding docs) I am going to volunteer sci to review the javaodocs as he loves doing so

[rain tartan]

i will ready the magnifying glass, my lady

just say the word

[twin wedge]

and after I finish reviewing it... then I get to go back to reviewing my own code and addressing todos... https://github.com/mekanism/Mekanism/pull/7925

[buoyant patio]

are the reviews for this far enough and fixable enough to target next weekend for this (so exactly 1 week after registries)

[raven coral]

I think it would be nice to get a bit more review

[buoyant patio]

we have pup implementing mek with it... I think the only thing that can expand on that would be a mod like curios

if mek and curios work, most other mods are covered too

[raven coral]

I kinda want to try porting AE2 to it

[buoyant patio]

does AE2 use other caps/systems to mek?

[raven coral]

technically I could port Applied Mekanistics too 😛

[raven coral]

not necessarily but it's a good test target I think?

• I'll have to do the work anyway once the rework is out

[buoyant patio]

can we push this to a feature branch and build an artifact?

[raven coral]

we should just store the artifact in github actions

there is no reason to have it on TC, IMO

[buoyant patio]

but gradle can't use it from there

[raven coral]

you can also build it locally and publish it to maven local

[buoyant patio]

yes but ease of use

[raven coral]

I just don't want it on the maven in the middle of the other "real" releases

[stoic warren]

TC wouldn't publish it to maven either

[buoyant patio]

we already have the networking stuff there so...

[stoic warren]

it what

[raven coral]

the networking stuff doesn't compile

hmmm apparently it did once

[timid cargo]

[Jump to referenced message](#builds message) in #builds

minecraftschurli referenced · Nov 14, 2023, 10:50 AM

NeoForge

Success

Build number

20.2.8-beta

Build branch

feature/1.20.x/networking

Quoted · Nov 14, 2023, 10:50 AM

[raven coral]

ok it's not on the maven

[stoic warren]

it's not no

i was almost worried

[buoyant patio]

@craggy kelp can you also give this a look

[twin wedge]

maybe? I hopefully will finish leaving feedback on it today, but not sure how long it will take tech to get to all my comments

[raven coral]

not too long hopefully but I keep finding other things to keep me busy -_-

[twin wedge]

I still need to do further testing with mek on it... just because I compiled got into game and changed code to fix the spots that still were expecting/requiring tiles (for transmitters). Doesn't mean that I tested all yet. Still need to test the radiation stuff to make sure that entity attachments actually work (but after what I had tested I fixed focus to the code review as I had at least a basic understanding of the system after rewriting a few thousand lines of mek

granted yes by the weekend after this one I definitely will have time to have done so assuming nothing major comes up in life

[twin wedge]

there we go

[rain tartan]

@twin wedge I at your "Shouldn't this be != n and the comment be a n-th time?" series of comments

[twin wedge]

I mean am I wrong?

[rain tartan]

probably not (I haven't checked it myself), but it is still funny to see

[young oyster]

This doesn't have a merge timeframe yet, right? Starting to debate making a barebones version of CM6 to see if any woes are solved with the rework

I'd want to see a mod like entangled, CM6, or hyperbox handle proxying >.>

[raven coral]

Probably 2-3 weeks

[raven coral]

Still didn't get time to go through the review 😢

[buoyant patio]

well it is probably only going to get more because C4 is doing one too

[raven coral]

That's fine, once registries are good I'll blast through the comments 😛

[raven coral]

one thing: the non-allocating lambdas should be kept as-is because a single instance will be cached by the VM

i.e. going over x -> true multiple times will always return the same object

[twin wedge]

ah

[twin wedge]

tech can you give me a ping after you end up rebasing this against the registry rework stuff?

[raven coral]

yeah will do

[shadow spruce]

Hello, I have a question about this rework: would it be possible to differenciate between an automation and a player interacting with a slot? For example, a hopper under a furnace will extract everything in the result slot, but also an empty bucket in the fuel slot, but nothing else. In the other hand, a player should be able to extract every items in the fuel slot. I'm asking this because I think that with the current system, it's not easily feasible? I saw in the PR that there will be 2 caps for item handling, but I didn't know if you could differenciate easily... Thanks!

[raven coral]

the capabilities usually exist only for automation

the PR adds an exception for entities because they are commonly used both for automation and for "direct" inventory access

for a standard block, it is up to the block implementer to implement player-facing filtering via Slot and automation-facing filtering via IItemHandler 🙂

[shadow spruce]

Ah I see, I did something wrong with my block 🙃 thank you!

[cerulean kernel]

the PR adds an exception for entities because they are commonly used both for automation and for "direct" inventory access
people do that all the time with block entities too, i don't understand why entities are a special case here

the common practice is to make the 'null' side capability unfiltered, or implement player-facing filtering on it

though fundamentally there's no reason the gui should have to access via the capability api at all

[shadow spruce]

Yes, but in a block entity thing, you need one unfiltered cap, then another for automations for side, that "wraps" the unfiltered one?

That's what I implemented, but it looks cumbersome

[raven coral]

that is how people generally do it

[twin wedge]

Null is historically “internal” so in lots of mods it is a read only view

At least for BEs

[shadow spruce]

The thing is that it's weird to say that when the menus for block entities expect itemstackhandler, no? Unless that's not the way to do menus

I'm new to modding, sorry if I don't really understand everything 😅

[shadow spruce]

Also, I had to create a wrapper myself and override the extractItem, maybe a neoforge-provided wrapper would be good? I know I could have used CombinedInvWrapper, but it does more stuff than just that 🤔

[raven coral]

what happens with IItemHandler is only tangentially related to this rework

[raven coral]

this is more or less what is intended, yeah

[shadow spruce]

Fair enough

[mint current]

Your BE can expose the IItemHandler through a getter which you can use to access it in your menu. The lookup through getCapability() is mainly relevant for other mods to access your BE's inventory without depending on your concrete BE implementation

[shadow spruce]

True, but I would still need an IItemHandler unfiltered, and one that has some functions overriden that I give in getCapability in any case, right?

[mint current]

Yes, if you need filtering on the exposed one, then you still need a wrapping handler for that in some way. I always make my main item handler unrestricted and then wrap it in simple filtering handlers for exposing to other mods (and sometimes also the UI, for example if the player should be unable to place an item in the output slot of a machine through the UI)

[shadow spruce]

Okay, makes sense!

Thanks everyone (also sorry for being a bit offtopic)

[cerulean kernel]

i still want to know what this "exception for entities" is all about

[raven coral]

Then read the PR 😛

[twin wedge]

super super tldr: hoppers can interact with minecarts and chest boats

[cerulean kernel]

ok but why should that be an 'exception'?

(it seems like the real problem is that we've implemented capabilities on vanilla entities that hoppers don't interact with)

anyway does someone have a link for the PR

[mint current]

See pins

[cerulean kernel]

ok wait a damn minute

"exposes the full inventory of some entity. " is problematic because it doesn't provide a way to determine the semantics of each slot

and i assume we're still not actually doing anything about the inventories of villagers, allays, etc, and still treating the equipment as a 'full inventory'

and why do we need to use capabilities for this, code that wants to interact with the equipment of a random living entity can use the vanilla API and not worry about some mod's entity having replaced it with something where slot 0 isn't the main hand etc

[raven coral]

You can use whatever

The point is to have code that is generic over any entity

If your entity is living you can use the vanilla methods (which the cap forwards to anyway)

[cerulean kernel]

ok but i don't think it's useful to load up the capabilities of generic entities with semantics that anyone who does anything else with their entities is going to break

[raven coral]

Please look at the full PR description first

[cerulean kernel]

if anything, the capability should only apply to non-equipment inventory i.e. the chest contents of horses, the extra inventory of villagers etc

I'm looking at it

there's nothing here about allays or villagers, and half my point is that the original implementation of horse stuff was half baked

[raven coral]

It is convenient to be able to just shove an item into an entity, wherever it lands

That's what the capability is for

Forgot about alleys, maybe they make sense to support in some way

[cerulean kernel]

[by villagers i'm referring to the food/seed slots of villagers - the same slots exist on pillagers but are unused, and i think piglins also have something going on]

[severe axle]

The vanilla entity inventory caps have generally been garbage

[cerulean kernel]

the fact that villagers' main hand is accessible via the capability but tends to contain the displayed item stack from a trade they're advertising [i think it's not even a copy, so mutating the stack could change the trade] could also be problematic

[severe axle]

It only makes sense for entities with "real" inventories, i.e. chest minecarts

[cerulean kernel]

which is why i don't like the fact that the PR notes seem to be codifying it

having separate automation capability, ironically, probably makes it even more complex than it already was to do the things that people have always done with the few mods [pneumaticcraft, integrated dynamics] that have historically actually supported automating entity inventories.

[quaint yoke]

Shouldn't capabilities on entities only expose stuff that's exposed to automation in vanilla? Like, what's the issue with that?

If hoppers don't interact with it, it shouldn't have a capability, right?

Unless some mod wants to add one itself or something but that's different

[cerulean kernel]

wait

does the new entity automation capability not support direction at all? what if i want the sides of an entity to actually be distinct to automation?

[quaint yoke]

Let me read through the implementation to make sure I know what's going on before I comment further

[cerulean kernel]

[some entities, like armor stands, may be axis aligned. Most entities at least have a top and bottom]

[quaint yoke]

"sides" of an entity? Issue is that it's entity-specific to what degree it has sides. An entity could have 12 sides you all you know, if it's a dodecahedron

[cerulean kernel]

yes but a hopper has six sides.

[quaint yoke]

Yes but the entity doesn't

[shadow spruce]

Could be side, top and bottom at least?

[quaint yoke]

A hopper is a block

[quaint yoke]

Cool, say my entity rolls

[cerulean kernel]

yes but the purpose of the capability is for hoppers to interact with it

an entity can decide that it cares whether the hopper is above it or next to it, or not.

like, the direction doesn't represent "side of the entity", it represents "where is the hopper in relation to the entity"

[quaint yoke]

Yes but with what degree of precision

[cerulean kernel]

[yes this still causes problems with entities larger than one block, which is why i wanted hit test context]

[quaint yoke]

At that point you'd be better off supplying the entity with a relative position vector or something

Which I would say is far more sensible than a direction, if you really want to go that route

[cerulean kernel]

iirc everyone else wanted contexts to be something that could be looked up in a hashmap, so vectors got vetoed.

[quaint yoke]

Then I certainly wouldn't do a direction

[cerulean kernel]

i don't want to relitigate all this at this point, i just wish we weren't losing things from the status quo

[quaint yoke]

If the requirement is enum-like, there is no good context that can be provided

Okay, went over the PR. Seems there's two different types of entity item capabilities

Including one for the full inventory

Why does that need to exist?

After all, we don't expect full inventory to be exposed for block entities

Or rather, I'd say if we expect it of one we should expect it of the other

[cerulean kernel]

horse.getCapability(ForgeCapabilities.ItemHandler.ENTITY)
Honestly, I find this concerning because of the fact that the old horse inventory handler didn't distinguish the saddle and armor slot from the rest

[cerulean kernel]

ok hang on

actually we do expect full inventory to be exposed for block entities

or i should say, Jade and The One Probe expect that.

[quaint yoke]

Sure, but many mods wouldn't want to expose it through the normal one

As that's used for automation

(the null direction too in practice)

[cerulean kernel]

historically that's what the null direction was for

[quaint yoke]

Yeah and it sucked

[cerulean kernel]

[using the null direction for automation was regarded as rude, but if you're really worried you can make it a read only view]

[quaint yoke]

Because suddenly if you had to do automation and didn't have a context...

You were kinda screwed

[cerulean kernel]

under what circumstance do you not have a context?

[quaint yoke]

When your automation isn't happening from another block

[cerulean kernel]

[honestly, i'd probably default to inserting from the top and extracting from the bottom in that case - pretend you're a hopper]

[loud flower]

that is not the purpose of the capability, the purpose of the capability is to allow mod interoperation

[quaint yoke]

That's what mods ended up doing, but it's a really bad reuse of the context in my opinion

[cerulean kernel]

i'm talking about, specifically, the puirpose of the ENTITY_AUTOMATION capabiltiy

[loud flower]

alright, my bad, I mistunderstood what you were referring to 😅

[cerulean kernel]

ok how did you designate the target? By clicking on it? you clicked on a side of the block, right?

though

now that i t hink about it

one case i can think of where you really do need a read/write view and don't have any face reference is the rftools storage scanner

[quaint yoke]

"push or pull items evenly from every block within <designate cuboid of block>"

[cerulean kernel]

most storage systems require you to 'attach' storage blocks somehow, but the storage scanner just uses a radius

[quaint yoke]

There's a million different cases I can think of where that sort of thing would occur

Having the null context be some "special" behavior is a bad idea. The null context should only mean "we have no context"

It shouldn't have semantics beyond that

Those should be reserved for, you know, different capabilities

(heck, that's half the issue the old entity stuff ran into)

[twin wedge]

upon closer inspection it seems that composters are still cursed

[raven coral]

[raven coral]

FYI if you need a different context you can just use a different capability atm

[twin wedge]

I am inclined to agree that for the automation entity itemhandler cap (but not the normal entity itemhandler cap) we may want it to have a direction context (even if none of our defaults actually return different handlers due to it), as it really doesn't hurt anything to provide the extra context

[raven coral]

the usual question is: what if the entity rotates?

[twin wedge]

hmm

part of me almost wants to say, custom nullable context:

• up
• down
• side
  But yes theoretically then there is rotating such as when swimming/crawling that an entity could be rotated along a different axis yet...

But especially if for none of the vanilla entities we actually need the extra context... We could still do Direction (so that we then can pass it from VanillaInventoryCodeHooks for hopper integration) and leave it up to the person implementing the capability provider on the entity to solve the question of how to go from Direction to which one to provide. As if they don't care (like we don't for the vanilla ones), then they just always return the same handler rather than actually making use of the context. Given one use case I can think of (that isn't even hard to figure that translation out) is a custom minecart that behaves differently for hoppers under the minecart or on specific sides than it does for ones above the minecart

so my vote would be: Add a @Nullable Direction context to the entity automation item handler, pass the proper direction via the hopper hooks that queries the cap... and then leave it up to modders to decide how they want to make use of that context in the capability provider they are writing

[buoyant patio]

another suggestion was a vector

[raven coral]

that's incredibly impractical 😛

[raven coral]

yeah I think that is the best option

[twin wedge]

Tech: instead of making a supplier and capturing it why not just use the already captured level and pos and query level#getBlockState where you call supplier#get?

[quaint yoke]

Yeah that's my point. For item capabilities specifically, it's really an item interaction capability. It shouldn't be a "automation, except this special behavior for the null context where it's not clear, and if you have no context and need to do automation you're screwed"

[raven coral]

yeah you're right - done

[quaint yoke]

If there's a use case for a different "items in the thing" capability, that's not automation focused, that should be a different capability not some special semantics attached to the null context

[raven coral]

also sorry for the review order I am addressing comments in a random-ish order (i.e. whatever I feel like addressing at a given moment 😄 )

[raven coral]

that's what the PR is doing already?

[quaint yoke]

Basically I'm saying I like what you did for entities splitting it in two, but the issue still exists for BEs to my understanding

[twin wedge]

That’s fine. If you notice the order I left comments… they were all over the place as well and different review sessions sometimes even had comments on the same file

[quaint yoke]

Which is that people have two different uses for null context: "viewing not automation" (which your entity stuff separated), and "automation but I don't have a direction"

But either way, I'm liking the PR a ton

[twin wedge]

Will you need me to add this as a comment to the PR at some point or do you have a note of it somewhere for yourself?

[quaint yoke]

Don't have the time to give it the full review it deserves but there's lots of people with more experience with capabilities than me

[raven coral]

I can add the comment right now, I was planning on doing it but it slipped out of my mind

[shadow spruce]

Would it be offtopic for this PR to include a ItemStackHandler simple wrapper, so each mod that want to do filtered-automation doesn't have to create one? I will maybe make a PR for the documentation to explain this as well, because it seems important but there's no documentation on how to do it anywhere

[light oriole]

I think something like that would be a good thing to include in the MDK

I kinda want to turn the MDK into a more complete example of things a modder might want to do, like living documentation in a way

[shadow spruce]

I think it's a good idea, but there should also be an empty template imo

Like the empty MDK, and a repo which is a "complete" example of a mod?

[light oriole]

That'll have to be a discussion for another time and another thread 😅

[shadow spruce]

Yes, true 😆

[raven coral]

Yeah it's a bit off-topic 😛

[shadow spruce]

Will wait for your PR to go through and then I'll make a request 😄

[craggy kelp]

It looks good to me, I was able to implement everything that Curios needs using this rework.

[raven coral]

@craggy kelp were you able to implement an entity IItemHandler capability under a custom namespace?

in principle that would allow mods to access curios slots without a dependency on curios by just referring to the capability

[craggy kelp]

I tested just re-implementing the Curios interfaces. However, I just coded a quick test run of what you said and it does seem to work. I'm able to implement an entity IItemHandler capability under my namespace and my test mod can access the items in Curios slots without any direct reference to Curios.

[raven coral]

Very nice 🙂

Thanks for checking

[quaint yoke]

Oh man, being able to access stuff like curios from other mods without a dependency on curios would be amazing. More benefits of the new capability system I guess

[cerulean kernel]

slot type context might be useful too

and were we still gonna try to make a more expressive inventory tick API? I remember that being a discussion last month or so when LexForge made an incompatible change to theirs

[raven coral]

Blergh I kinda hate the idea

[buoyant patio]

how is this going? are we planning this or next weekend for finishing? (even if we postpone the merge to 20.3 does not mean we have to wait for it to be ready)

[raven coral]

I'll have more time for review feedback tonight

[raven coral]

ok let's try going through more comments

we will keep customPersistentData

[raven coral]

OK that's all for today probably

addressed a lot of comments

most comments are small tweaks, which make me think that the PR is in a pretty good state

[twin wedge]

Marked some more of my comments as resolved based on your responses. I will try to look at the changes you made in the next few days as well as address some of the more complex responses you gave (such as re-evaluating some stuff about the canQuery stuff so that I can figure some stuff on my end out about usage, and if that usage is actually proper). But definitely seems like a lot of the remaining comments are mostly small tweaks

oh and I will also try to do some better testing... in regards to actually test the entity attachment I have and the entity capability that interacts with said attachment. And then also test item capabilities, as all I have actually tested so far is item handler caps on blocks/block entities

oh and cc: @rain tartan https://github.com/neoforged/NeoForge/pull/73#discussion_r1403776377 give your input on how you think we could go about documenting that thing. Yes it isn't technically written as javadocs yet (and the PR isn't necessarily quite ready for a full javadoc pass by you as a couple of the comments relate to adding some minor javadocs to some methods, but that is all just details)

[raven coral]

(for level attachments I was thinking of preserving their full functionality if we still want it, including saving on the server side; but it seems a bit pointless - anyway got to sleep, thanks for the comments)

[twin wedge]

oh and part I got distracted from that I meant to say in the above in regards to me trying to take a look in the next few days... hopefully I will be able to, though life is also a bit in a weird spot currently.. so it is definitely possible that it will turn out I am not able to set asside the time

[buoyant patio]

no stress I'll add the final RFC label on monday so you have a whole week

[raven coral]

We don't have to merge this on a weekend specifically

[rain tartan]

i shall defer this to later™️ -- need to read through the PR before I understand the problem at hand

[twin wedge]

Hmm. For BlockCapabilityCache should we validate the position is loaded when retrieving the capability if the cache isn't valid and set the cached value to null if it isn't? (Or add a secondary getCapabilityIfLoaded type method that then has that behavior). Given while cleaning up my handling of some stuff related to the BlockCapabilityCache I realized that realistically when it gets invalidated from a block unloading, instead of just ignoring the change I should make use of the fact that it is null so I can skip it as a connection on the pipe network...

So ideally instead of having a Level#getCapability call that then will cause the position to be loaded it would just return that there isn't currently a cap there, as the cache will have the invalidation listener get triggered again when the chunk is loaded again... Plus I think theoretically that means currently it is inclined to sort of ghost chunk load due to querying the cache for an unload to cause it to load, potentially firing the invalidation listener again (and then shortly afterwards again as the chunk will then unload again)

And the reason for doing it at that level instead of a caller managing that... is as callers have no way to know if the cache is valid or not they would have to query if the position is loaded for every call to getCapability even if it is still valid so we know it hasn't unloaded

[quaint yoke]

An issue I could imagine is that now you've got a null value cached

[twin wedge]

why?

when the chunk loads

it will fire the invalidation listener

and clear the cache

at which point no more null value is cached

[quaint yoke]

Okay that makes sense then. What's the current behavior with unloaded chunks?

[twin wedge]

if you query a BlockCapabilityCache for an unloaded chunk (such as if you have a tile on the border of an unloaded chunk and are querying the neighboring one), then it ends up loading the chunk in order to query the block state and tile at the position of the cache

(which then given the ticket level of the chunk will be queued for unloading, which then will fire the invalidation listener for the cache all over again and potentially cause mods who are checking if there is still a capability accessible there to start the whole process over again of querying the cache on an unloaded chunk)

[quaint yoke]

Hmm, I can imagine that not being optimal. However, I also think that consumers should be able to distinguish between "who knows" and "no capability here" - but I suppose they can just check if the chunk is loaded so maybe not a big deal?

[twin wedge]

that is why I said potentially depending on viewpoint make it as a second method on the cache, as the consumer then can pick which one to call

[quaint yoke]

Yeah I really like the splitting the method option

Because then the behavior is very clear and you get to pick which one you do

No reason to trigger chunk loading if you don't need to after all

Though honestly I think both solutions could work

[raven coral]

Personally I don't really see why we should protect against "chunk not loaded" but not "chunk is not fully loaded"

I would say that this sort of ticking check is the responsibility of the mod doing them - simple BE tickers will always have adjacent chunks loaded

[twin wedge]

I mean I guess, but the way said mods would have to deal with it is basically copy pasting the block capability cache to add one extra check. Hence why I feel like it may be worth adding natively or as a second alternative method on the class. As yes for simple tickers it may not affect them, but also the extra check (that only would add an extra check when the cache isn’t set yet) shouldn’t cause those cases problems either? So what is the harm in having it if it helps the more advanced cases without harming the simple/basic case?

[raven coral]

Ideally you only want to query chunks that are ticking or adjacent to ticking chunks

The fact that the chunk is loaded or not doesn't help you there

For MI I think we originally had a check for ticking chunks, but in the end it was causing performance issues and I had to implement a custom system that kept track of ticking chunks that contain pipes or something - so modders writing pipe nets will probably need custom handling anyway

[twin wedge]

<@&1128776937410670663> can we get some more opinions on this? (Messages started here: #1135328752658829312 message)

[severe axle]

It should be null if the area is unloaded imo

[twin wedge]

Yeah rather than loading, firing an invalidation check, then unloading and firing another invalidation check. Like just have the cache usage get null for unloaded chunks. And for direct calls to level for get capability have modders be responsible for handling not accidentally loading chunks. Just like get blockstate and get block entity they have to

[dapper pecan]

seems reasonable

[raven coral]

Kind of weird to special case one of them IMO

But idk

We have apparition notifications now so maybe it isn't as bad

[twin wedge]

Yeah, that was part of my thinking. Given the null will get invalidated anyway when the chunk loads…

[raven coral]

Do we want to allow querying boundary chunks?

We could do it "properly" and only allow querying chunks that are ticking or neighbors of ticking chunks

Basically what you said with listening to chunk status changes (which I believe you added an event for)

[twin wedge]

I mean we could, but personally I think it is enough for now to just level#hasChunk it. As for if people do care about the ticking level of chunks they can go ahead and create their own cache similar to our provided one… the main reason to have a loaded check is to prevent chunk throttling and just being loaded and unloaded repeatedly, without requiring modders to all write their own caches

[raven coral]

I don't really see how one is less problematic than the other

Repeated chunk loading is annoying, but querying too close to boundary chunks is also problematic

[twin wedge]

Why?

[raven coral]

Because you might cause block updates etc

[twin wedge]

Meh, which in general will still behave just fine. But also like invalidation happens on load and unload… if you start returning null for wrong ticking level then you need some way to make sure modders invalidate their stuff properly when the ticking level changes, rather than just being able to use the remove/clear removed stuff

[raven coral]

We can invalidate caps when the ticking level goes over or below a threshold

But that feels kind of weird

I don't like having just the isLoaded check, it doesn't feel motivated by actual use cases

[twin wedge]

That feels like overkill and a pain to keep track of

[raven coral]

Surely networks already check that the chunk is ticking, not just loaded? At least that would match hopper behavior

[twin wedge]

Mek’s doesn’t (at least not really for outputting to, for pulling from it does). Whether it should? Not really sure and don’t currently have the brainpower or emotional energy to care enough to try and figure out if it should continue to behave that way.

And then if you think about networks like at least how enderio worked in 1.12 where it could have the network go across unloaded chunks of the other side was loaded, that sort of even more so doesn’t match that concept (granted yes maybe the other side is ticking I don’t know but still)

[raven coral]

Not sure, MI (which has networks that go across unloaded chunks) checks for ticking chunks

But OK we can add a check and see what happens

Can you leave a comment on the PR?

[twin wedge]

yeah I can

and I mean yeah maybe checking for ticking is best for mods to do, but in general there are so many mods that don't even check themselves against calls to other blocks potentially causing chunks to load that I would suspect at best on average modders might handle unloaded chunks in their pipe networks, but almost certainly are not going to handle ticking chunks any differently than non-ticking chunks

[raven coral]

there's only a handful of high quality pipe mods anyway ^^

[quaint yoke]

Just noting, performance wise repeated chunk loading is far more worrying than updates in loaded but non ticking chunks, all things considered. Chunk loading can be rather expensive

[iron flame]

Is the capability rework coming in 1.20.2 or 1.20.3? I’ve been waiting on this to update my mod since I heard its going to break existing worlds.

[twin wedge]

Pretty sure we changed the target to 1.20.3 (potentially on release of 1.20.3)

[iron flame]

Good to know, thinking I’ll make an alpha release for my mod for now and update it later to a full release when it happens

[raven coral]

<@&1128776937410670663> level data attachments are basically superseded by SavedData - do we want to keep them nonetheless?

[cedar oar]

has something changed in SavedData in 1.20.2+?

[quaint yoke]

Nah, it's been this way for ages

Level data attachments hasn't been that useful in a long time

[cedar oar]

the reason we had level caps before, was to be able to use the same API and interfaces

I assume that's not relevant to data attachments

[quaint yoke]

That's my understanding

[stoic warren]

no

[cedar oar]

or I guess the method for attaching is different, but is that enough reason to have them? cos level caps are a massive hack

I vote no, not worth keeping

[raven coral]

you can't attach an API to levels anymore

in the rework

[quaint yoke]

I'm not a maintainer but I am also a proponent of yeeting them (and in fact in my own stuff I preferentially use saved data already)

[rain tartan]

saved data is on both sides?

[raven coral]

the question is whether you should be able to attach storage to them or not

[raven coral]

no, server only

[hallow ledge]

data attachment on the client doesn't make sense and you can simple sync stuff

[cedar oar]

what why not, that was useful. I have at least two mods which attach an API (no data) to level (the data is per-chunk, but the API takes care of accessing the chunk for you)

[raven coral]

Then make that a static method that takes a level

[cedar oar]

well yeah but that's less pretty :P

ohwell

(that said, I'm not sure why "you can just use a static method that accepts a level" would be justification for not having API attachments for levels, when the same can be said for anything else)

[raven coral]

how is having a useless .ifPresent prettier

[cedar oar]

doesn't the redesign remove that?

but that said, the same way anyone would want to do block.getCapability(ITEMS) instead of ItemCapability.get(block)

anyhow I have looked at my code again

and I basically have a static method already

the API isn't being used publically in this mod at all

the world capability is a queue of actions to be processed on tick, so not actually API I suppose

huh I was certain I had actual API using level caps

turns out the two mods that use AttachCapabilitiesEvent<Level> are both data

I definitely had ideas for level API

but I have to assume I never actually published any

[raven coral]

I might just yeet level data attachements for now

and add them back later if people really need them

so we know

[cedar oar]

if they are completely yeeted I will have to reconsider these two features I have, because both store runtime data in level (non-serialized so SavedData isn't the right use case)

one of them I think can be done by other means, because it's a special spawner logic and I believe vanilla has a way to attach those

[raven coral]

technically you can store non-serialized data in SavedData but it's a bit gross 😄

[cedar oar]

https://github.com/gigaherz/EyesInTheDarkness/blob/master/src/main/java/dev/gigaherz/eyes/EyesSpawningManager.java

I have no idea how that works, but someone pointed it out to me a while ago and I was like "neat, but the code is already written so 🤷‍♂️ "

hmmm ServerLevel#customSpawners is an immutable list

my other use case is this: https://github.com/gigaherz/ElementsOfPower/blob/master/src/main/java/dev/gigaherz/elementsofpower/cocoons/CocoonEventHandling.java#L103

temporarily stores BlockEntities to be added to the tracking table, since the BlockEntity load methods are not safe to access Chunks in

[raven coral]

I mean yeah that is the sort of use case that I have in mind that is not nicely handled by WSD

[cedar oar]

hmmm since those are server-side only, I could just use a static queue

and process it in server tick

[raven coral]

yeah

[cedar oar]

so if I didn't forget any mod, I can live without Level data attachments or Level capabilities

[raven coral]

👍

[raven coral]

almost done with the review comments 😄

[raven coral]

what about this?

[twin wedge]

Edit pr description and add to potential future work?

Given old system didn’t really support it either

[raven coral]

👍

Should we invalidate caps on level unload? IMO that could be problematic and probably isn't really needed

(that basically means server shutdown?)

[twin wedge]

seems like overkill, especially given vanilla doesn't unload levels anymore right?

[quaint yoke]

Mods might still dynamically unload levels

So I would to be safe

What's the problematic stuff that could arise?

[twin wedge]

though maybe mods like CM would have it be useful to them? As they naturally have caps that go across levels. cc: @young oyster

[rain tartan]

i see no reason why not to invalidate on level unload

[twin wedge]

because mods may have side effects in their listeners

that then maybe world doesn't save fully properly? idk

though not sure that is actually a problem

[raven coral]

If the listener itself is in a level that's being unloaded...

[twin wedge]

I honestly thing people can use the unload events?

and just listen themselves

[quaint yoke]

Hmm, should be fine if it's invalidated early enough?

[twin wedge]

as odds are the cap invalidation for those are highly specified anyway

[quaint yoke]

The worry is that someone ends up with a cached, never invalidated reference to a capability that is invalid as it's in a level that no longer exists

[quaint yoke]

And I mean, this is true of chunk unloading too, right?

[raven coral]

Hmmm yeah actually that's a good point 🤔

[twin wedge]

except it is more straigthforward and logical in your listener to check your block is still loaded

than knowing if a level is mid unload or not

[raven coral]

Are people actually gonna check that?

[twin wedge]

idk

[raven coral]

I think it's harder than you think

[twin wedge]

I do

[raven coral]

We had countless related bugs in AE2

(e.g. sending a block update in a chunk that's being unloaded... and then you end up with two copies of the chunk)

[rain tartan]

as a smol tangent, this kinda reminds me of that idea/PR of providing a removal reason for block entity removals

wonder if that's still needed by mods

[raven coral]

Yes it is

was that PR merged? I hope so

[twin wedge]

aka the one that lex cutdown

[rain tartan]

iirc it wasn't merged

[raven coral]

Wait wtf

It's super useful...

[rain tartan]

https://github.com/MinecraftForge/MinecraftForge/pull/8303 this

at least there's a comment about why it was closed

[raven coral]

hmmm yeah maybe onRemove works

not sure heh

[rain tartan]

i didn't think much of it, since I didn't know enough about the usecases myself

[raven coral]

whatever

[rain tartan]

wait a minute -- I though this was #neoforge-github

whoops

[raven coral]

back to the issue at hand

what do we do with these level unloads

what about chunk unloads even

[mint current]

Pretty sure Raoul, the dev of RefinedStorage was a major proponent of this, might be a good idea to ask them whether that's still needed

[raven coral]

I think he has a workaround (checking if the unload method is called before the remove method)

[mint current]

Yeah, that's ugly though

[raven coral]

Exactly

It's also kinda dumb because unload could probably be replaced by checking the removal reason

[mint current]

Yup

[raven coral]

I think that invalidation listeners in the same chunk are a bit problematic right now

Depends on what the listener does ofc

Surely this was also an issue with the previous system?

[noble rampart]

would a level unload unload all chunks first?

[raven coral]

there's a chance it doesn't fire any chunk unload event

[noble rampart]

it's mostly relevant to dimension mods like RF dimensions I guess 🤔
But I can see it

I am not sure if RF dimensions can actually be ever removed

[raven coral]

I suppose we could fire the unload event for each chunk

that would be more sensible IMO

[rain tartan]

i know you mean "a chance" as in, "it might or it might not, I haven't checked"
but I chuckle at the thought of it being actually controlled by some random coin flip, ergo "a chance"

[dapper pecan]

they can be

I believe McJty reimplemented that functionality to work with modern dimensions

Hyperbox also does that iirc

[raven coral]

I think we can just try the current status of the PR and see what happens

That's my general philosophy with decisions that have an unpredictable outcome

Just ship it and we'll fix it later 🤷

[young oyster]

CM does not unload its levels but having the listener tell me when to stop routing stuff would be great

Because I'm doing OW/Nether to CM quite often, I'll be doing chunk listening and other shenanigans often

That said, cm6 is going to be radically changing the concept of tunnels anyway, so.. wishlist

Leaning harder into a room being its own storage/buffer, and having capability networks. So, I'd need things like "this cap is shared among these 9 chunks, only invalidate once they're ALL unloaded"

[twin wedge]

ignoring the fact I think I maybe partially broke it at some point when fixing some other thing so need to look at my code further... I think the main way currently to in general have the cache know the block it is at is unloaded is:
a. WeakReference to the BlockEntity
b. take advantage of the fact that BlockEntity#onChunkUnload fires for all the BEs before BlockEntity#setRemoved (which is where the cap invalidation happens), so you can mark a boolean or whatever in your BE that you are no longer loaded and then by the time things are invalidating are fine

GRANTED I don't know ordering how this ends up being affected by block caps on blocks rather than BEs (for example the composter)

but also similarly to how you pointed out the other day to me on the PR... that the point of not allowing requerying the cache while invalidation is going on is in case things aren't fully setup... so people should queue the stuff some. If people queue it then ideally however that is queued would be superseded by the fact the thing that should be holding the cache is no longer loaded itself

so maybe it won't be that big an issue anyway

[raven coral]

yeah I think we can just try it and see what happens™️

[twin wedge]

one thing lex did say in general that he was definitely right about is there is only so much you can do to keep modders from shooting themselves in the foot. So regardless there are going to be some mods that have issues and a lot that are just fine

like at most we may want to add some note to the cache and invalidation listener things to mention that "edge" case of when the listener is being unloaded as well so that modders can think about how they want to handle it

[raven coral]

added a note

do we want to invalidate caps on level unload or not, then?

if anything the chunk unload event should fire (or be made to, if it doesn't yet)

[raven coral]

amazing 😄

@buoyant patio FYI

[twin wedge]

Make the returns an immutable copy in those new javadocs as a new immutable copy

So people know more easily each call isn’t just a simple view getter

[raven coral]

that's what the currently known is for

[twin wedge]

I know, but still that could still imply it is a view of the backing map say collections unmodifiable map

How that wraps the actual mutable one

Without needing a new object each time

[raven coral]

alright

[twin wedge]

Hence my suggestion to make it slightly clearer imo

Also @rain tartan the pr is probably ready for a javadoc pass (I only skimmed them while reviewing the code and stuff)

In the next few days I will try to track down a bug I was noticing when loading worlds that I am 99% sure is in my code, but I want to verify it isn’t actually on the reworks side. I also will try to do some basic testing of other features that I have ported to new impls, but I haven’t gotten to testing yet

[rain tartan]

javadocs, you say

[raven coral]

please review the PR so it can be merged in the initial 1.20.3 release

😉

[buoyant patio]

or in 20.2 (depends on how big the next pre / rc version is)

[raven coral]

I'd still prefer 20.3

otherwise we risk the stability of 20.2

[buoyant patio]

20.2 is not stable

[raven coral]

it is reasonably stable now

there is a high chance that the cap rework will have some bugs that will take a few weeks to notice

and if we release it on 1.20.2 now we'll have to choose between fixing them (massive waste of time) or not fixing them (and leaving 1.20.2 in a more broken state forever than it is right now)

[rain tartan]

@raven coral why does your PR lack the build status

[raven coral]

the merge conflicts stop the action from running on the merged PR

(I think)

[rain tartan]

ah

wait

that shouldn't be a factor

[buoyant patio]

it is because the checks are not run on the branch but on a branch that is the PR merged into the target

[rain tartan]

ah. I was thinking of the wrong thing

for future reference, https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request

• Workflows will not run on pull_request activity if the pull request has a merge conflict. The merge conflict must be resolved first.

  Conversely, workflows with the pull_request_target event will run even if the pull request has a merge conflict. Before using the pull_request_target trigger, you should be aware of the security risks. For more information, see pull_request_target.

anyway, fix conflict

[raven coral]

I'll fix it but it shouldn't stop you from reviewing the PR

[buoyant patio]

sooo... how long do you think this last pass will take @rain tartan @raven coral?
can we target this weekend?

[raven coral]

we should target 1.20.3

[cedar oar]

I wonder if we'll see a pre4, or rc1 next

[buoyant patio]

if we get a pre4 we definitively should merge this on 20.2

[keen prairie]

I'd guess a rc1 unless anything major has appeared

[buoyant patio]

I was mainly asking to get you to look at the thing xfact pinged you about in #neoforge-github

if this is done then you should move your attention to getting the stable release done

[raven coral]

there is no rush

I think we'll get the 1.20.3 release Tuesday next week

[buoyant patio]

wouldn't that require an rc1 before friday

[raven coral]

probably yeah

[hallow ledge]

pre4

[cedar oar]

hahaha I didn't expect one today

[quaint yoke]

rc1 before Friday is looking pretty likely then

[cedar oar]

yeah, at this point I'm expecting rc1 on Thursday/Friday, and release Monday/Tuesday

[buoyant patio]

eh probably thursday, tuesday

[twin wedge]

did a tiny bit more testing of a couple other of the systems... and what I tested works fine (unsure about other things)... And the weird thing I was running into is I am almost certain is on my side... why it didn't used to be a problem in the past I have no idea, and why it even exists how it does currently I also have no idea as some of the logic just seems super off... but that is my problem not this PRs

random other comment though to confirm... we don't need to care about neighbor changes at all anymore for consuming capabilities right? (And we don't need to fire them when our capabilities change? More specifically should we fire them or stuff or intentionally remove that related code?)

[cerulean kernel]

pipes may want to connect to blocks that have capabilities

er doesn't mekanism do that? how would that work with no neighbor change stuff

[quaint yoke]

The consuming block would have a capability invalidation callback. That will fire regardless of whether it's neighbors or half a level away

[cerulean kernel]

how do you notify your neighbors that you've grown a capability?

e.g. by changing the side I/O configuration of a machine from 'none' to some other value

(yes, blocks further away might be interested, in principle, e.g. a red stringed container type block that can proxy a block it has line of sight to. i don't know how to solve that. maybe a global 'capability availability change notify' event?)

[quaint yoke]

That's still invalidation. And there's still nothing special about "neighbors" here

Invalidation is just "whatever I thought was here is no longer what's here", whether the capability has grown or disappeared or whatever

[cerulean kernel]

so does that mean that something that wants to attach to any capability that any adjacent block may gain needs to register itself to listen to all six invalidation handlers of all six neighbors?

[raven coral]

Yes that's correct

[raven coral]

Yeah no need for neighbors

[cerulean kernel]

that seems like it would add a lot of bloat

like, something has to store all those callbacks

[raven coral]

it's actually quite cheap

[serene minnow]

I haven't looked into the new system, but if a block goes from no cap to a cap, there's no invalidation?

[raven coral]

there is

(in the new system)

[serene minnow]

Guess I'll need to spend some time reading it. It just sounds weird as the block doesn't know a neighbour can even have a cap?

[quaint yoke]

Well that's the thing. Anything could in theory have a cap, right?

If you see "oh, that's a block" it's impossible to tell, really, whether that block might one day have a capability or no

[raven coral]

yeah you just create a cache and give it a listener - "if this block ever has a cap just tell me"

[quaint yoke]

But when you check if it has a capability, you get a lookup into the capability cache

[quaint yoke]

Which, yeah, that - it lets you add a listener

[serene minnow]

So instead of rechecking when a neighbour updates?

[quaint yoke]

When a neighbor changes its capability, it'll trigger the listener

(if something holds a listener to that block position in that level)

[twin wedge]

only if they cache it

if you aren't caching it you are just querying every time you use it

but then you don't care about the neighbor changes anyway

[cerulean kernel]

no, the target block (or something anyway) has to keep a reference to you to call your callback.

and this isn't about caching, it's about stuff like "neighbor doesn't have a capability so my pipe doesn't connect in that direction, but if it gets one i want to know about it so i can change the pipe connections"

[raven coral]

forge takes care of that already

[cerulean kernel]

ok but that's six references to the callback in six different places, per block in a pipe system.

[raven coral]

you just listen to a specific position, and the block will invalidate listeners at that position

[raven coral]

yes, and?

[cerulean kernel]

i'm just saying it seems like that would add up

each position's listener list is, what, at least sixteen bytes for a bare bones singly linked list, plus whatever the size of the callback object itself is

[raven coral]

we'll see if it's an issue in practice

[twin wedge]

and the main reason I was asking was given IBlockExtension#onNeighborChange sort of feels like based on docs still potentially should be fired when caps change

[raven coral]

I would say no because it's very annoying to fire 😛

unless it gets done automatically with setChanged

[twin wedge]

actually idea. You want to modify the docs for that method as part of this PR to mention changes like it does now, but reference (using {@link ...} ) that for cap changes the invalidation stuff should be used (like invalidating and also the listener stuff)?

[raven coral]

yeah why not

[twin wedge]

just remember there are docs on both the blockstate extension and the block extension

[twin wedge]

Do you want me to add a comment on the PR about it? If so I will get that a bit later

[raven coral]

yeah please

should I port the PR to 1.20.3-rc1 already 🤔

[buoyant patio]

not yet i did not rebase the porting branch yet

[raven coral]

should we do "port to 1.20.3-rc1" followed by "capability rework" followed by the (very small) "port to 1.20.3"?

just so we are sure that we can release everything on "day 1"

[buoyant patio]

you can just rebase onto the squash later since there should be no diff between after the rebase and after the squash

[raven coral]

true

[twin wedge]

We still need javadoc review cough @rain tartan cough

[rain tartan]

uh, whoops

i'll get right on that
...after I finish porting my mod locally

[buoyant patio]

no now

[rain tartan]

could someone please fix that merge conflict so the check runs on the PR and my mind is at peace

[buoyant patio]

tech

[rain tartan]

the pending check icon keeps throwing me off

[raven coral]

so how is the javadoc review going?

surely the merge conflict was not a blocker 😉

also @buoyant patio I added final RFC

[twin wedge]

of course it is a blocker because it disturbs sci too much to be able to focus on actually reviewing the code

you also still have that unresolved convo with schurli on it (and the general comment/change request that I left about docs... and whatever sci comes up with)

[raven coral]

I know but that's 5 min of work to resolve

[twin wedge]

mine yes

schurli's no idea

[raven coral]

I think we just don't fire it for now

and see if we need to add it later

I was planning on just reworking the comment a bit

[twin wedge]

ah okay

[raven coral]

something like "Right now we don't invalidate on level unload, but maybe this is worth looking into."

[twin wedge]

yeah. Until we have more mods in play where:
a. we may have some mods adding listeners across levels
b. mods that are dynamically unloading some levels
we don't have a great use case example

[raven coral]

yeah exactly

and the mod dynamically unloading levels can easily work around it by unloading each chunk one by one, which is probably good practice anyway

[twin wedge]

though speaking of given now I am kind of curious... @manic oar do things like rftools dims in modern versions even bother with unloading levels while the server is actively going?

[manic oar]

No because as far as I know all existing dimensions are always loaded

[brazen verge]

dims are always loaded

chunks aren't

[manic oar]

Yes

[twin wedge]

that is what I partially assumed, I just wasn't sure if maybe some mods like that did stuff to effectively force unload them in cases where you potentially have a ton of custom dims

[brazen verge]

there's not a huge load for loaded dims that aren't the overworld

[raven coral]

I think Hyperbox dynamically unloads levels

[brazen verge]

it just finalizes any queued block/fluid ticks & lighting updates

all the entity ticking is skipped entirely

so they mostly just get left alone now

[twin wedge]

so then yeah tech I think it isn't really a problem at all. The only real case is that if people want to do some cleanup logic when the server is shutting down and then save said cleanup stuff... which to be honest is likely massive overkill and if it actually affects anyone we can figure it out then, and if it is even really a valid use case (or if is something the mods should handle in a different way)

[brazen verge]

I still think they should be unloaded, personally
But mc is mc, so whatever

[twin wedge]

cc: @fleet gull, can you confirm?

[fleet gull]

yes

rips 'em out of the dimension registry and the loaded level array

or rather infiniverse lib does that and hyperbox just asks infiniverse to do it

https://github.com/Commoble/infiniverse/blob/main/src/main/java/commoble/infiniverse/internal/DimensionManager.java#L208

need to remember to save the level on unload in 1.20.2 so hyperbox doesn't have to do it manually

[raven coral]

do you unload every single chunk as well?

[fleet gull]

that's a funny question

I assumed if I yeeted the level then the chunks would eventually get GCd

[raven coral]

I meant to ask whether you fire any chunk unload event

and whether you should 😛

[fleet gull]

[raven coral]

if you don't you might have to fire some cap invalidation events instead

[rain tartan]

everytime I open the PR and see that yellow check, I impulsively scroll down to the bottom to see why the check is yellow, even though it's been the same reason for the past handful of times I've opened it

[raven coral]

[twin wedge]

I semi agree with sci it is potentially worth doing a rebase on it to fix the merge conflicts so that we can have the tests and stuff run before you port it to the 1.20.3 branch where the tests wouldn't be running

[raven coral]

If I PR it to Kits actions will probably run

[twin wedge]

@rain tartan how goes the review

[rain tartan]

i knew I was forgetting something

[twin wedge]

that's what pings like that are for!

[rain tartan]

@twin wedge @raven coral habemus review

[raven coral]

just finished addressing the last comments - I left as few as "unresolved" for you to double-check but in principle I consider everything addressed and will commit the PR to Kits tonight (in about 6 hours)

@silent @stoic warren @twin wedge @buoyant patio @rain tartan ^

apparently silent is broken sorry

[stoic warren]

silent only works if you do it right the first time lol

[raven coral]

rip

[twin wedge]

Sold

To the rework collecting matrix guy

[raven coral]

Who's collecting all the reworks?

[twin wedge]

Neo

[raven coral]

oh ffs 😄

[raven coral]

https://pr-21.neoforged-website-previews.pages.dev/news/20.3capability-rework/

current draft for the blog post

[fleet gull]

I'm confused, can chunks have capabilities or just data now

[raven coral]

only data

[brazen verge]

maybe specify that this is only true for serializeable attachments lol

some silly people might get confused over this

[raven coral]

thanks, will add that tomorrow

[brazen verge]

Im' curious as to why we're still keeping the invalidation approach

when it's been pretty consistently proven to be an ineffective and unviable system to use historically

modders just don't invalidate stuff themselves anywhere even remotely close to enough to be reliable enough to use

[quaint yoke]

The average modder doesn't have to invalidate anything

The only case where you have to invalidate something yourself is if you've got the same BE, and it changes which actual capability instances it provides - which is a pretty rare occurrence

[quaint yoke]

On the flip side, the system that requires that invalidation - the block cache system - has some pretty big potential benefits performance wise