oh ig the item wouldnt
#1.21.5 Snapshots
proven compass
tho all it did was random.setSeed(seed);, no?
in 1.21.1 at least, it might have changed
between the versions
but u could set the seed urself if ur still passing the random
marsh mural
i mean that would be a bandaid fix but id rather find the correct place to apply the seed
rather than just patching it into the client levels random source
i mean vanilla is applying it somewhere, so i should be doing the same when i render stuff through my placement logic
my guess is its moved somewhered in the BlockModelPart collection stuff
to bake the seed into the collected model parts
rather than applying the seed each time some quad is rendered
but i cant seem to see anything seed related in there
ahh i think i see it, vanilla pre seeds the random source before invoking BlockStateModel.collectParts
rather than seeding it during quad renders
so i should maybe be doing the same before i collect parts for my render
frail patio
Yes, the vanilla method only accepts parts, which themselves do not depend on randomness, nor does any other part of that method
fleet dome
You can confirm that and go ahead
you can also just set it to ignore incompatibilities in the launch tab and save that, no idea why it doesnt like that specific tex param setting
lunar saffron
You can confirm that and go ahead
As I said, I did confirm it, otherwise I wouldn't know the specifics about that incompatibility. I didn't dig further into it yet, the only thing I know is that the early window is breaking things
lunar saffron
I'm not saying the early window is breaking the capture, I'm saying the early window is breaking Fabulous somehow.
Also, as soon as the frame debugger is present, it works fine regardless of the early window being enabled or not, so I'm not sure the frame capture is even gonna tell us anything
spice geyser
ayy
warm night
1.21.5 RC
Changelog: https://www.minecraft.net/en-us/article/minecraft-1-21-5-release-candidate-1
SnowMan: https://github.com/neoforged/Snowman/commit/30de37264101dcd9fd2f0b010228ed4e029a2fd3
Primer: https://github.com/neoforged/.github/pull/14/commits/cd21ed32b422bab24ec2ed656674575f94e77a30
-# Changes: <>
Slicedlimes Videos:
- Main: https://www.youtube.com/watch?v=8sPih4fHc-8&pp=0gcJCU8JAYcqIYzv
- Data/Resource Pack Changes: N/A
halcyon bane
cinder saffron
Looks like they're almost done, awesome!
warm night
i think someone needs to kick the showman... @oblique tusk
oblique tusk
https://tenor.com/view/patrickkicking-patrickkick-patrickfunny-patrick-spongebobkicking-gif-18165246
spice geyser
1.21.5 before the end of the month?
oblique tusk
btw dpeter, you realize you too can kick Snowman (as can any maintainer)? 
warm night
well it is not there currently ....
oblique tusk
you don't see this?
oh wait
you're not a maintainer
I forgot
alright then, continue with the pings
oblique tusk
btw for the peeps and lurkers, if you like low-stakes games regarding when the next MC version is published, go check out #1250829990195626115
proven compass
saturday
mystic gulch
https://github.com/neoforged/Snowman/commit/30de37264101dcd9fd2f0b010228ed4e029a...
Why is this repo private?
warm night
That is a link to our internal decompiled source repo. It has nothing you can't recreate on your own machine, but making it public would be illegal as it would re/distribute the source code
it just helps neo development so we can link to code lines / changes easily
real bolt
There's several alternative tools also available although I can't name them off the top of my head
They're all mostly the same, decomp and commit each release as a new commit to a git repo
fleet dome
I'm not saying the early window is breaking the capture, I'm saying the early wi...
ok we're mixing stuff but i did have early display and NO frame debugger and it worked π
lunar saffron
weak wasp
marsh mural
# 1.21.5 RC
**Changelog**: https://www.minecraft.net/en-us/article/minecraft-1-2...
im kinda mad at myself, that ive been late to 2 snapshots in a row
i was doing quite well up until now, cant be late to the next one, gotta get on top of this
at least peter posted a similar message and pinned it 
warm night
yeah it is the same template as what you were doing
but don't worry we can share the load of making the pinned posts
LOL
lunar saffron
Resolved the tnt and dispense behavior rejects. I opted to give IBlockExtension#onCaughtFire() a return value to forward the return value of TntBlock.prime(). This will also allow modded blocks to "deny" a flint-and-steel interaction from a dispenser which prevents damaging the flint-and-steel.
lunar saffron
ok we're mixing stuff but i did have early display and NO frame debugger and it ...
I'm even more confused now than I was last night.
I just started the normal Neo client in Kits and Fabulous worked fine. Then I started the test client and Fabulous was broken. Last I started the normal Neo client again and Fabulous is broken there as well now.
Restarting the GPU driver (i.e. pressing Win+Ctrl+Shift+B) fixes it for the first start of the game after the restart, subsequent game starts are once again broken.
If I disable the early window, then it works fine no matter how often I restart the game. Interestingly enough, enabling the early window again after starting at least once without it breaks it on the first start with the early window instead of the second one.
fleet dome
The hell is going on. The only difference would be that I am on a slightly older driver version hm.
lunar saffron
Apex also mentioned running into Fabulous being broken last night
@marsh mural are you using an Nvidia GPU and, if so, which driver version are you using?
Btw, this is what the loading overlay looks like for me after handover when the early window is enabled 
lunar saffron
I do get GL errors for that: [01:04:44] [Thread-3/INFO] [mojang/GlDebug]: OpenGL debug message: id=1281, source=API, type=ERROR, severity=HIGH, message='GL_INVALID_VALUE error generated. Invalid offset and/or size.'
That's where this ^ stacktrace comes from
fleet dome
Hmmmm
marsh mural
<@201070433280196608> are you using an Nvidia GPU and, if so, which driver versi...
im running a rtx 3060 on zorin pro 17
and i belive my gpu drive version is 570
but the issue with fabulous i was having fixed after i disabled early display in the config like you mentioned xfact
-# in ./config/fml.toml
left image - earlyWindowControl=true
right image - earlyWindowControl=false
marsh mural
Btw, this is what the loading overlay looks like for me after handover when the ...
i am also getting same as you btw, with early window enabled
proven compass
what is this you're working on?
I didnt realize neo was injected into rendering so much
lunar saffron
We're trying to understand how the early progress window manages to break Fabulous mode rendering on certain systems
proven compass
the screen that only shows at the beginning of launch affects gameplay rendering?
lunar saffron
It shouldn't but somehow it manages to fuck up the GL state or the GPU driver to such an extent that it does break something
proven compass
love that
marsh mural
huh thats interesting, the glass isnt rendering with level geo but my placement renderer renders it fine
-# the glass your seeing is my placement renderer, its adjacenet to the same block in world
the only thing im rly doing diferent to vanilla render is using my own entityTranslucent render type (which is a copy of vanillas with no depth test)
and using a vertext consumer wrapper to apply my own alpha to the render (similar to creates ghosting)
lunar saffron
The thing you are doing differently is that you're rendering directly to the main render target. The thing that is specifically broken is texture-to-texture blitting, which is why specifically Fabulous mode and the glowing entity outline are broken.
floral mesa
Has anyone tested on Linux/Mesa already or is it worthwhile for me to try?
lunar saffron
Another test subject certainly won't hurt
lunar saffron
@fleet dome I may or may not have found the issue. The VBO that should be bound is 1, the VBO GlState thinks is bound is 1 but the actual VBO bound at this moment is 3. This is captured from a breakpoint in the GL debug message handler
fleet dome
Yeah something like that sounds likely, question is: why? did I grab the GL state wrong?
lunar saffron
You only grab and snapshot the GL state in DisplayWindow#render() which is only called from NeoForgeLoadingOverlay after the handover is completed. This GL error happens in DisplayWindow#renderThreadFunc() though which does not capture and snapshot anything
I found the culprit and the reason why it worked previously: RenderSystem.initRenderer() prepares and uploads the RenderSystem.QUAD_VERTEX_BUFFER that's used for certain full-screen blits. The underlying buffer has, to absolutely nobody's surprise, the buffer ID 3. This didn't happen on 25w09a because this buffer was previously created on demand instead of eagerly on renderer init
Doing this (first and last line added by me) in RenderSystem.initRenderer() fixes it.
fleet dome
ah yes, well not exactly what we want to do ig. π
but alright explanation found at least
lunar saffron
Now it also makes perfect sense why specifically Fabulous mode and the glowing entity outline get rekt by this: the former is a post-processing shader which uses the RenderSystem.QUAD_VERTEX_BUFFER via PostPass and the latter uses it via RenderTarget#blitAndBlendToTexture()
lunar saffron
ah yes, well not exactly what we want to do ig. π
Absolutely, I just wanted to confirm it
The only thing that still really confuses me is why it worked fine on the first game startup after restarting the driver or entire PC
fleet dome
That that is also very confusing (and why did it work for me) π
gleaming pendant
ok let's have a look
lunar saffron
@gleaming pendant you've got a visibility patch in there that will bite you in a moment: https://github.com/neoforged/Kits/blob/435f57aa1f28fec24d1ec0290abf895051f81278/patches/net/minecraft/client/renderer/RenderStateShard.java.patch
gleaming pendant
ah yeah I checked that it builds but then I realized I forgot to gen patches
heh
it will be for after The Rebaseβ’οΈ
lunar saffron
π
gleaming pendant
Port to 1.21.5-pre3
Co-authored-by: Apex <[email protected]>
Co-authored-by: coehlrich <[email protected]>
Co-authored-by: embeddedt <[email protected]>
Co-authored-by: Matyrobbrt <[email protected]>
Co-authored-by: Minecraftschurli <[email protected]>
Co-authored-by: Sara Freimer <[email protected]>
Co-authored-by: Sebastian Hartte <[email protected]>
Co-authored-by: Technici4n <[email protected]>
Co-authored-by: XFactHD <[email protected]>
btw quite some people contributed to this port π
drifting finch
```
Port to 1.21.5-pre3
Co-authored-by: Apex <[email protected]>
...
Yes
gleaming pendant
(I only checked the primary authors not the commit messages π€·)
drifting finch
Very nice work!
gleaming pendant
doesn't look too bad
lunar saffron
Speaking of contributors: maty still hasn't fixed the gametest registration in the test framework
gleaming pendant
yeah well that will come for later then
we still have two rejects from a few weeks ago
lunar saffron
Yeah, those two are particularly fun...
gleaming pendant
I'm not touching them
if someone has issues with fluids they can feel free to have a look
lunar saffron
Part of one of the rejects is related to the damage pipeline and shield blocking. I would not be surprised if that missing hunk would cause a crash with the damage container stack because there might be a push or pop in that hunk But nobody has tested taking damage yet, so nobody has run into that potential issue yet
gleaming pendant
as usual these patches will need to be reapplied by hand; marking that down for now
ok I made a mistake, let's start again
sterile abyss
Speaking of contributors: maty still hasn't fixed the gametest registration in t...
shhh
oblique tusk
file an issue so I can assign maty to it and occassionally join in on the stabs
gleaming pendant
<@298082405745426433> you've got a visibility patch in there that will bite you ...
the rule only disallows access widening
lunar saffron
Is there any reason to not disallow narrowing access?
gleaming pendant
gleaming pendant
Is there any reason to not disallow narrowing access?
one of the transformers wants a private field
lunar saffron
I'm aware of that transformer but do we actually have any case where that means narrowing a field's access?
gleaming pendant
it looks like the BucketItem#getFluid method is gone π€
lunar saffron
That's already the case in 1.21.4. The transformer in question isn't used on BucketItem anymore since the registry order allows passing the Fluid directly instead of wrapped in a supplier
gleaming pendant
ok this one still exists
lunar saffron
Hmm, unfortunate
limpid crypt
Is the patch for Sheep.java correct?
lunar saffron
Yes
gleaming pendant
Hmm, unfortunate
I can add a special case for it
lunar saffron
True
gleaming pendant
apparently I missed a compilation failure coming from the rc1 port
lunar saffron
Yeah, I'm fixing that right now
gleaming pendant
I was gonna force-push it π€
I guess it doesn't matter; just keep in mind that there likely won't be a further squash
lunar saffron
I was gonna force-push it π€
Sure, works for me. Do you have a patch for it ready or should I give you mine when I'm done?
Ideally a patch that doesn't just use LevelRenderer.BrightnessGetter.DEFAULT π
gleaming pendant
no patch no
dark wasp
I guess it doesn't matter; just keep in mind that there likely won't be a furthe...
tfw your dad grasses over the garden because he doesn't want to maintain it anymore
lunar saffron
π
lunar saffron
alright, pushed
You forgot the ModelBlockRenderer patch change
lunar saffron
Thank you
thin cairn
Mojang will remove it eventually
It doesn't do any harm atm
getSecurityManager is just implemented as return null; now
olive rover
java 24 is currently sobbing
java 24?
wasnt the latest 22
awh shit java upgrade again
olive rover
awh shit java upgrade again
may package manager bless us
i hate doing this manually
fallow merlin
I don't think that matters afaik
MC is currently using Java 21
And not much Neo can do about it afaik
gleaming pendant
Well we could patch out the SecurityManager
gleaming pendant
Yeah π€·
fallow merlin
Yea
fleet dome
@drifting finch Well I meant that as: the API to build/modify a pipeline object in isolation is not related to the system that applies such transformations based on some registered/idempotent transform
i.e. having the pipeline->builder methods is useful outside of that, too.
river canopy
I'm copying this code I wrote as part of the discussion we were having on the pipeline modification stuff:
RenderPipeline.Builder modifyPipeline(RenderPipeline.Snippet originalState, RenderPipeline.Builder builder) {
if (originalState.xyz())
return builder.withSnippet(COOL_CHANGES);
return builder;
}
That is completely true, but given how the API discussion went untill now, the transforms applied (from snippets directly) are not identifiable
So you would need to have some identifier applied no?
drifting finch
Well not applied
fleet dome
the transformation itself is an opaque Function<RenderPipeline, RenderPipeline>
fleet dome
which needs to have a contract attached to it: idempotency
drifting finch
It would wrap the API we are discussing here
fleet dome
and then we need to build a registry for those
river canopy
the transformation itself is an opaque Function<RenderPipeline, RenderPipeline>
UnaryOperator 
fleet dome
It would wrap the API we are discussing here
yes, or just return a custom pre-defined pipeline
depending on what it is doing
drifting finch
yes, or just return a custom pre-defined pipeline
Yes
But it would need to be idempotent and identifieable
fleet dome
We just need to set in stone: idempotency. Since our code would only ever call this function once per pipeline (based on its referential equality)
river canopy
I think UnaryOperator is expected to be indempotent
fleet dome
Yes, hence the need for a client-side registry of those, I think
java.util.function interfaces have no contract on what the functions actually do
river canopy
Interesting, doesn't Stream require a bunch of the functions/etc passed in to be indempotent?
fleet dome
We should be very explicit about it, since we're baking in the assumption
Well tbh, it wouldn't even matter all that much
the transform would only ever be called once
(for an input pipeline)
river canopy
Right
fleet dome
even if the function wasn't idempotent. it might just not do what the modder expects
if they thought they could do a if (Math.random()) < 0.2 { different pipeline state }
My idea was then to:
RenderSystem.pushPipelineManipulator(REGISTRY_KEY)
guiGraphics.renderItem(....)
RenderSystem.popPipelineManipulator(REGISTRY_KEY)
river canopy
Ah, I see now
fleet dome
It'd internally build a cache key out of the manipulator stack keys
There's essentially one central point at which a render pass gets created
river canopy
I see why you were thinking RenderPipeline->RenderPipeline rather than builder->builder
My thought was to have it be a one-and-done thing in the builder's build() method
fleet dome
You'd need a cache: Map<Pair<List<ResourceKey>>, RenderPipeline>, RenderPipeline>
river canopy
But that wouldn't work if you wanted to render one item with, for example, inverted colors, and then one without
(at least, my one-and-done approach wouldn't)
fleet dome
Oh, yeah no this needs to be ambient state to essentially "wrap" render calls
you could wrap the push/pop into a :
RenderSystem.renderModified(STENCIL_MANIPULATOR_KEY, () -> {
guiGraphics.renderItem(...);
});
But the lambda allocations are always a bit... eh
river canopy
Yucky, but I'm not entirely sure if there's a better way that isn't just passing the pipeline all the way down
drifting finch
you could wrap the push/pop into a :
```java
RenderSystem.renderModified(STENCIL...
I like that API more
fleet dome
You can't pass a pipeline all the way down
You can offer both, but the lambda will incur allocations
re: pipeline passing.
if you just want to override stencil state, for example, you just don't know what the other state of the effective pipeline is going to be. since you cannot know which pipelines will be used as part of the guiGraphics.renderItem() call.
drifting finch
You can offer both, but the lambda will incur allocations
Yeah. I personally don't care for every single frame, I simply find that second lambda based APIs less error prone (aka forget to pop)
fleet dome
Imagine special item renderers, for example. they could use any pipeline they wanted to even multiple!
That's why you need that Function<RenderPipeline, RenderPipeline>
There are ways to build easier APIs on top of that, btw.
river canopy
you could wrap the push/pop into a :
```java
RenderSystem.renderModified(STENCIL...
If we're doing the lambda approach, we should make a variant which accepts a state parameter
fleet dome
If you simply want to override a few states, you can have that in a RenderPipeline.Snippet
And we can offer a simple factory method for a manipulator that takes in the snippet
and creates essentially a manipulator like this: pipeline -> RenderPipeline.builder(pipeline).withSnippet(snippet).build()
drifting finch
Yeah that would be great
river canopy
re: pipeline passing.
if you just want to override stencil state, for example, ...
Yeah, my thought was that renderItem(...) would effectively get a "modifier" parameter, and then when the pipeline gets used we can have a patch which pulls the already-modified pipeline from a LRU cache keyed by the modifier/pipeline pair
But then we'd need to patch everything to accept this modifier parameter all the way down
at the very least, if it's something like UnaryOperator, there's the andThen/compose methods to make applying them in order a little easier
We could then probably make it RP.Builder->RP.Builder and have the usage site manually convert it to/fro if a modifier was passed
fleet dome
Yeah, my thought was that renderItem(...) would effectively get a "modifier" par...
you cannot use a LRU cache
river canopy
Oh?
fleet dome
well there's two problems: a) i am using renderItem just as an example here, there's an arbitrary number of render methods that you'd want to be able to wrap
b) The GL render system permanently caches the compiled GL pipeline state for any RenderPipeline it ever sees
river canopy
Oof
fleet dome
That's why the manipulators must be registered and have identity
river canopy
Well, I guess that means a LRU cache is worthless then
I don't think they need to be registered and have identity if it's just a set of modifier functions called in order
drifting finch
I don't think they need to be registered and have identity if it's just a set of...
Actually you do need it
It is easy to fuck up else causing the GL System to create a new permanent cache entry on every single call
Sometimes multiple times per frame
That is a memory leak of epic proportions if multiple modders do this
river canopy
I imagine that the performance impact of such a mistake would be noticable even in dev environments, though
drifting finch
Depends on the usage and the place of the modification
So, not always.....
We already missed it here once
Took xfact trying it out for his blocks to notice
river canopy
Just to confirm, this GL cache is based on the object identity of RenderPipeline, right?
drifting finch
Correct
river canopy
Mkay
drifting finch
Create a new one, create a new cache entry
river canopy
I don't disagree that it's a problem to leak memory; I'm just not entirely sure the ambient state approach is necessary to avoid it though
Though I do admit it would make it a lot easier to make it "automagic" if multiple mods are interacting without requiring every modder to be diligent about passing their modifiers along
fleet dome
I don't think they need to be registered and have identity if it's just a set of...
They do, since people are inevitably going to shoot themselves in the foot by passing a lambda that'll have different referential equality every time
oh Orion already said that π
Yes, what Orion said π
river canopy
Yeah, I see the problem you're talking about
fleet dome
It's also 1000 times easier to debug if you can see that the active render pipeline was "entity/whatever modified by ae2:stencil_stuff"
drifting finch
oh Orion already said that π
I got ya....
Been disconnected with this port. But kept up with the concepts and issues so I at least knew that
river canopy
It wouldn't be a lambda anymore, but we could go for a half-way approach with some sort of PipelineModifierBuilder which lets you insert/preinsert named modifiers, but I guess at that point it's basically the same as the ambient state but as a parameter
hmmm
My main issue is that I'm just not a fan of ambient state, especially considering how Vulkan is setup and how the GL abstraction stuff seems to be setup for Vulkan in the future
river canopy
you could wrap the push/pop into a :
```java
RenderSystem.renderModified(STENCIL...
It kind of is, since this is effectively pushing to an invisible stack that modifies all render calls inside of it
drifting finch
It is ambient through the CPU side of the engine, but as soon as the pipeline is actually used by the engine to call the GPU it is a concrete static final object
river canopy
Yeah, that's true
river canopy
But I think we should respect that on the CPU side too
drifting finch
But that cache is also our issue
fleet dome
It kind of is, since this is effectively pushing to an invisible stack that modi...
That is unrelated to object identity
And unrelated to the cache
drifting finch
But I think we should respect that on the CPU side too
I am neutral on this. I want a good API. And the stuff sharte has shown to me feels natural to use
fleet dome
Without ambient state you cannot modify render passes that are being created by some other renderer you call
drifting finch
So I am good with it
fleet dome
Whether that ambient state is a stack on the rendersystem or a threadlocal doesn't really matter
drifting finch
Given that this api in particular will not be used by renderer mods but by content creators
So I am okey with that API not following the vk/metal/rendering architecture
fleet dome
This has really nothing to do with VK/metal
river canopy
Without ambient state you cannot modify render passes that are being created by ...
You could if the render calls required some sort of list of modifiers
river canopy
It wouldn't be a lambda anymore, but we could go for a half-way approach with so...
Hence this
fleet dome
You could if the render calls required some sort of list of modifiers
you are not directly calling a render call
you are calling some arbitrary method
that may create multiple render passes
maybe 10 stack frames down
river canopy
I'm saying that renderItem(...) as an example should accept a parameter which describes how to modify the render passes, and then the modder would flow that back up to wherever the most logical place is for them - e.g. if Screen#render got the same treatment, they'd just pass the parameter through
fleet dome
I am heavily opposed to that
That is just one method that may render something
And it's called in a thousand places
river canopy
Yes, I agree it's a lot of changes to make, but given everything I know about rendering, it "feels" more right to me compared to the ambient state
drifting finch
Not to me in my opinion
fleet dome
I don't see how this is rendering related
This is applied and resolved before anything is written into the fake "command buffer"
river canopy
Rendering doesn't start and finish at the command buffer; you've got the context and the whole pipeline
fleet dome
Sure, but this is not really different from a bunch of other context issues all over software engineering
be it tracing, correlation ids, rpc deadlines, etc. etc.
It's never nice to do this
river canopy
Yeah, that's fair
river canopy
Yeah, and I agree
I was just hoping that there was a better way, but I can't think of one
fleet dome
Well it was my initial idea but got reality checked by Xfact π
river canopy
I may not be an old man, but I feel a "old man yells at xfact" meme is appropriate here
river canopy
you could wrap the push/pop into a :
```java
RenderSystem.renderModified(STENCIL...
But yeah, I agree that something like this is probably the "least bad" approach. Maybe we should also add some sort of parametrised modifier system to alleviate some of the potential issues that could arise though (e.g. if you only want to invert/highlight the colors of one item when rendering a campfire in a UI element)
lunar saffron
I personally would significantly prefer the lambda approach over separate push/pop methods.
Another idea, inspired by a discussion on the Fabric Discord about this exact push/pop mismatch problem with PoseStacks, would be to return an AutoClosable object from the "apply pipeline modifier" method (can even be a re-used object, avoiding any additional allocations) which is used with try-with-resources and pops the stack when it's closed
drifting finch
I personally would significantly prefer the lambda approach over separate push/p...
I do that exact try-with-resources model in C&B for batch mutations
The closing of the batch object (which is an AutoCloseable) triggers the internal updates, to modeldata, models, structural outputs etc
It is a very nice pattern, but still allocates now and then, and is annoying if you have to stack multiple of them on top of each other or inside of each other which can make the code unreadable.
The lambda would shine better there
In my opinion
lunar saffron
The lambda would also have to be nested unless we provide an overload that accepts multiple modifiers (which I don't see much of a point in, just make it one modifier at that point), so the visual clutter is similar. As for allocations: in this particular case the AutoClosable object can be re-used since it doesn't actually hold any state, it just calls pop on the stack
river mural
the transformation itself is an opaque Function<RenderPipeline, RenderPipeline>
If we define that function can we use UnaryOperator instead 
lunar saffron
Another thought regarding this modifier idea: is there any practical benefit to having the modifiers be arbitrary functions? If we instead limit the API to only accept RenderPipeline.Snippets, then we can avoid the builder altogether for singular modifiers by applying the snippet in a direct pipeline->pipeline copy and only have to create a single builder when applying multiple modifiers instead of having to go pipeline->builder->pipeline->builder->pipeline. The snippets could then also be part of the cache key instead of having to register modifiers and putting their keys in the cache key
lunar saffron
@fleet dome I've got a present for you: https://github.com/neoforged/FancyModLoader/pull/259 π
fleet dome
Another thought regarding this modifier idea: is there any practical benefit to ...
Nah not enough, sadly
You have to be able to inspect which pipeline it is you are getting
And be able to return it unmodified
lunar saffron
Right, that's unfortunate. Would have been far too easy π
fleet dome
I.e. maybe you can only handle block vertex format
(not that I have checked whether the pipeline even encodes that)
lunar saffron
It does
fleet dome
It's not unfortunate, it's better imho
limpid crypt
Are we allowed to report bugs?
fleet dome
I want the registry also to avoid mistakes and get better debugging
drifting finch
Are we allowed to report bugs?
Yes and no
lunar saffron
That's fair, yeah
drifting finch
You are allowed to properly report bugs, with reproduction etc
Not just random screenshots of things that does not work
lunar saffron
Are we allowed to report bugs?
Yes, unless it's the one where you get a few GL errors during startup and Fabulous mode doesn't work
drifting finch
Yes, unless it's the one where you get a few GL errors during startup and Fabulo...
Also that
But if people start reporting stuff with just a screen shot, or a single line, along the contents of: "This does not work"
fleet dome
<@84313704874663936> I've got a present for you: <https://github.com/neoforged/F...
We should redesign the flow later since we do know when MC code first starts running and only need snapshotting then
Since it's relatively expensive
drifting finch
We should redesign the flow later since we do know when MC code first starts run...
Given that he only does it once in that PR
I considered it okey for now
lunar saffron
Given that he only does it once in that PR
Well, it happens every frame
fleet dome
That is once per frame if I am not mistaken
Tbh doesn't matter for the loading screen probably
drifting finch
Well, it happens every frame
Sure but mojang does that too, and in the grand scheme of things, this screen is only visible for a short amount of time
So it really does not matter
THis is a simple and well understandable solution
fleet dome
They never read the state from gl
drifting finch
Which is more important to me
lunar saffron
Sure but mojang does that too, and in the grand scheme of things, this screen is...
Mojang does not read back GL state, they only cache what they previously set. Reading back the state from the driver is what's expensive
fleet dome
That is what is slow heh
drifting finch
Mojang does not read back GL state, they only cache what they previously set. Re...
Yeah true
But you know
This suffices for this particular usecase
If it where me I would pin this screen to scenematic 24 FPS
fleet dome
It does yeah
drifting finch
And the rest is not something I would care for
fleet dome
Once per frame doesn't matter
limpid crypt
Is it normal that when the player uses elytra, the eye height is not updated? (I think it's not a Minecraft Vanilla bug)
drifting finch
Is it normal that when the player uses elytra, the eye height is not updated? (I...
Compare it to vanilla
And track the reason in the code
Then report it properly, with links to the code and a way to reproduce it
limpid crypt
drifting finch
Again I just told you, screenshots do nothing for us
fleet dome
How do you even find this stuff
limpid crypt
Compare it to vanilla
it's not in Vanilla
trail fable
is that vanilla or Neo, what is the exact difference, etc. etc.
please, can you just make complete issue reports
personally, I disregard any issue reports like this on my repo until I have the proper info
limpid crypt
ok, wait
Well... Discord isn't the best place to make bug reports, I'll make a bug report on Github
drifting finch
Yeah lolo, I am going to says this once: Write a proper bug report, or I will mute you from this thread, and the github org. We have told you this now so many times that I can not count it on one hand, and the way you are reporting stuff and asking for stuff, is always incomplete. That incompleteness causes us a lot time where we are chaising our tails, not supporting the community or helping the community forward.
So here is my official last warning for you: Start properly reporting stuff, or we will start taking counter meassures.
lunar saffron
<@84313704874663936> I've got a present for you: <https://github.com/neoforged/F...
Unless anyone has any oppositions, I'm gonna push the FML bump for this PR to the porting branch
drifting finch
Unless anyone has any oppositions, I'm gonna push the FML bump for this PR to th...
That is why I reviewed it fast and easy for you
So be my guest π
lunar saffron
Done
drifting finch
Thank you very much for you time to fix this!
lunar saffron
The fix was trivial for how much it took to find the culprit π
fleet dome
The fix was trivial for how much it took to find the culprit π
It's always like that with GL state mismatches /sigh
lunar saffron
Yup. But, the sync gl debug config once again saved the day
limpid crypt
Yeah lolo, I am going to says this once: Write a proper bug report, or I will mu...
is this correct https://github.com/neoforged/NeoForge/issues/2044 ?
drifting finch
is this correct https://github.com/neoforged/NeoForge/issues/2044 ?
The eye height looks completely identical on all three pictures.....
So I am going to say, not even by a longshot
trail fable
yeah, I can't see the difference either
lunar saffron
The issue is the view vector (the blue line that's only visible on the Neo screenshot), not the eye height
true turret
You have to be able to inspect which pipeline it is you are getting
to mirror the current stencil behavior, a push/pop of a modifer snippet would work
limpid crypt
oh
drifting finch
The issue is the view vector (the blue line that's only visible on the Neo scree...
I don't even know if that is a rendering issue or anything else
Which still indicates to me: This issue is not sufficient
trail fable
the view vector being wrong is curious
merry falcon
Could be a symptom of a larger issue. Worth investigation
drifting finch
For sure
But the issue is not sufficiently well described
At least for one made on a snapshot
merry falcon
oh
Basically the red line is eye height. Blue line is the look vector for the entity (where it is looking)
Wonder if size command affects this. Or swimming
trail fable
either way, worth investigating, but the bug report needs improvement
fleet dome
to mirror the current stencil behavior, a push/pop of a modifer snippet would wo...
Well what we have now mirrors current stencil behavior already
true turret
and works counter to the design of RenderPipeline
fleet dome
Which doesn't matter at the moment
But if we redesign it, we should not half ass it since the invest is very high for that use case alone
true turret
instead of a globally applied modifier snippet, could a map of (base) RenderPipeline -> modifier RenderPipeline.Snippet be whats pushed?
ehh, well, that doesnt solve the identity issue
true turret
Its not the fastest thing, but we could hash the RenderPipeline except its name
if done ahead of time in the constructor, there would be no performance impact for any statically created pipelines
drifting finch
Its not the fastest thing, but we could hash the RenderPipeline except its name
In that case we should patch in equals/hashcode
true turret
yes, doing thats what i meant to do
ie: make a record out of it, but ignore the "location" field, because the GpuDevice doesnt really need to care about that
drifting finch
Unsure about that
true turret
its used for error messages and debug labels, functionality/what-will-render it doesn't matter
^ one it hits the GpuDevice, PostPass does something with it and shader pre-warming also uses it to check that everything got compiled
drifting finch
^ one it hits the GpuDevice, PostPass does something with it and shader pre-warm...
Yeah for sure an option
Right now though, your idea feels a bit "depth-less"
Like yes it fixes the identity issue
But I seem to miss how this fixes the identity issue
Well
Yes it fixes it by making the Device etc properly cache the stuff
And not rely on object identity i guess
Which would open up a much easier path to "mutability" of the pipeline
true turret
which means we can safely dynamically (allow mods to) create/transform RenderPipelines
drifting finch
As you could just transform an existing pipeline
and with it the new instance would get a new proper identity
And that identity is the same between all pipelines and modification combinations that result in that pipeline setup i guess
@fleet dome or @lunar saffron is anyone of you aware of whether you researched this kind of approach (patching in a proper equals / hashcode, either pre-computed in the constructor, or on the fly), and if yes why was it not chosen, if not what do you think of this approach.....?
true turret
i'd suggest pre-computed or lazily-computed to avoid re-computing it for static untransformed pipelines in the general case
drifting finch
i'd suggest pre-computed or lazily-computed to avoid re-computing it for static ...
Lazy computed brings no advantage IMHO
99% of pipelines will be immediatly used
true turret
multi-step pipeline transforms
drifting finch
Unless the situation arises where once modder transforms an already transformed pipeline
Yeah
But how often does that happen
true turret
thats the only case where it will make a difference, and transformed in the first place is likely to be relatively rare
lunar saffron
<@84313704874663936> or <@165621075642679297> is anyone of you aware of whether ...
I don't think that option was considered
drifting finch
I don't think that option was considered
Somehow this solution of adding the identity compute seems like a too easy solution and we are missing something.....
true turret
immediate downsides i can think of are that it creates more garbage and requires computing the identity properly (which also might be slow)
lunar saffron
This took far too long but I found the reason why the view vector is wrong: beds, or more specifically the entity's "bed orientation".
drifting finch
Wtf, what have beds todo with the view vector?
lunar saffron
LivingEntity#getBedOrientation() is supposed to return null when the entity is not in a bed. However, our patch to support custom beds that are not BedBlocks returned UP in those cases. This caused the player renderer to overwrite the eye height (which is later used as an offset for rendering the view vector) with that of the player in a standing pose, regardless of the player's actual pose
drifting finch
Oef
lunar saffron
Looking at the porting PR failures, I just noticed that we are currently not doing any mod loading in GameTestMainUtil.runGameTestServer() which is called from the gametest server Main
trail fable
damn
lunar saffron
Next order of business: vanilla now stores a "started cleanly" flag in the Options. Right after the options are initially loaded, that flag is set to false and the options are written to the file. This causes modded keybinds to be wiped from the options since mod loading only begins significantly later. I'm not sure there is any good way to solve this other than moving mod loading before the init of Options and either firing the RegisterKeyMappingsEvent right after Options init and reloading the relevant parts of the options file or firing the event right from the Options constructor before the initial loading
gleaming pendant
As expected
oblique tusk
lunar saffron
Next order of business: vanilla now stores a "started cleanly" flag in the `Opti...
At least in my test case in a FramedBlocks dev environment, this solution seems to work fine
fleet dome
Und ne das war noch ErkΓ€ltungsbachwirkungen
Alles fine aber fast 40min beim.superkato in dem Kabuff da.runzustehem war scheisse
lunar saffron
Wrong channel
oblique tusk
I see the phrase "40 min". combined with German, I can only presume it's talking about the DB
not the SQL kind either
drifting finch
superkato....
That is a name i know from a restaurant
Not sure if that is what he is refering to
oblique tusk
Und ne das war noch ErkΓ€ltungsbachwirkungen
@fleet dome just in case
fleet dome
Somehow this solution of adding the identity compute seems like a too easy solut...
No, identity comparisons are used in multiple places
and RenderPipelines are large
So I don't really see a point in undoing that part of mojangs design
which is based on renderpipelines being long-lived static objects
I already outlined how we can add a modifier system
- registry for
PipelineMutator(which would beFunction<RenderPipeline, RenderPipeline>) - our stack in RenderSystem (or command encoder) takes in
ResourceKey<PipelineMutator> - we cache eterenally in one of the rendersystem classes:
Reference2ObjectMap<ResourceKey<PipelineMutator>, Reference2ObjectMap<RenderPipeline, RenderPipeline>> - when the stack isn't empty, it becomes a trivial:
setPipeline(RenderPipeline p) {
for (var transform in stack) {
p = transformCache.computeIfAbsent(transform, ignored -> new Reference2ObjectMap()).computeIfAbsent(p, transform);
}
... continue on as before
}
Just from memory / pseudocode, that's how I'd do it
drifting finch
No, identity comparisons are used in *multiple* places
Args well yeah
Then we are out of luck
drifting finch
Yeah true
fleet dome
They do a if (this.lastPipeline == pipeline) { noop, no state changes }
that is extremely fast
drifting finch
Yep
true turret
if (this.lastPipeline.identity == pipeline.identity) would also be equally quick
once that identity is computed
lunar saffron
Next order of business: vanilla now stores a "started cleanly" flag in the `Opti...
@fleet dome what's your judgement on this ^ and the proposed solution below it?
gleaming pendant
I say move mod constructor earlier and fire an event immediately before the init of Options
fleet dome
Next order of business: vanilla now stores a "started cleanly" flag in the `Opti...
I mean is this just one more reason to finally move the ctors to before Minecraft init? π
ONCE AND FOR ALL!
π
(let me quickly check where the options are loaded)
true turret
`if (this.lastPipeline.identity == pipeline.identity)` would also be equally qui...
arguably, would be faster than reference equality even for long-lived render-piplines if two identical pipelines end up being created, as they would have identical identities
fleet dome
Okay it's in the Minecraft ctor, so finally moving it to before that will solve this too
I'd say let's just finally do that in 1.21.5
gleaming pendant
But why would that happen?
fleet dome
Why would what happen?
oblique tusk
i'm tempted to ask someone to file an issue on the repo for that, so we have a record of it happening and why
true turret
multiple identical piplines?
fleet dome
That can obviously happen
But I don't like the complexity of introducing interning
Every patch is paid for in sweat and blood on the next snapshot
true turret
multiple identical piplines?
someone doing something like forcing on/off a state that is the only differentiator between two pipelines, then you get two identical pipelines created in different ways
gleaming pendant
multiple identical piplines?
Aren't they interned?
fleet dome
Mojang just public static final's their pipelines and uses object identity
gleaming pendant
We might be able to make the ctor private and inject a factory method instead? To intern new instances
gleaming pendant
Well I should actually look at this heh
fleet dome
We can intern them ourselves transparently
Oh yes, wait, forget it
RenderPipeline have an ID
we're not going to intern anything π
I'd rather have the ID reflect the transform if possible
lunar saffron
I'd say let's just finally do that in 1.21.5
Sounds good to me. Here's v2, still works in my test (the stupid LogicalSidedProvider really needs to die)
fleet dome
I have to check that, gimme a bit
Ok so you dont need to actually put that after the render thread init
and we need a bit more plumbing
gimme a sec
We're back on the main repo now, right?
lunar saffron
Yes
lunar saffron
Ok so you dont need to actually put that after the render thread init
I just put it in the last possible place before the MC ctor Β―_(γ)_/Β―
gleaming pendant
Sounds good to me. Here's v2, still works in my test (the stupid `LogicalSidedPr...
We can just kill it, no?
fleet dome
Sooo
Is this the point at which we kill those
and move them from FML to NF
*with the exception of FMLConstructModEvent funnily enough
gleaming pendant
why is that needed now? π
btw we still have 3 days to split neoforge if we want to pursue that
fleet dome
Ah wait, I actually mistook those for the ones we're going to add to
fleet dome
btw we still have 3 days to split neoforge if we want to pursue that
we really should :_|
We should still move those, they're not in the right spot there
unless we actually go ahead and do the fabric loader thing of splicing in the event emission in FML itself
rather than patch it in NF
proven compass
split neo? wouldnt that be best for a major version
gleaming pendant
it mostly just means moving a lot of files π
trail fable
btw we still have 3 days to split neoforge if we want to pursue that
not again 
first Forge -> NeoForge, now NeoForge -> NeoNeoForge
this is getting out of hand
gleaming pendant
that's not what split means π
trail fable
I know, but let me make my jokes 
trail fable
I already contribute nothing else
fleet dome
And change it everywhere in the code π
drifting finch
We are not going to split now
The architecture and design for the split is not documented and discussed among the maintainers
We can do that for the next version
fleet dome
eh Orion, I think it was discussed to death really
What Tech is talking about is just the source set split in NF itself
not the artifacts
fleet dome
and how that has to work is prescribed by Mojangs split upstream
drifting finch
But not now
fleet dome
it has to happen at a breaking point
drifting finch
Yep
drifting finch
I suggest one not 3 days before a release.....
gleaming pendant
tbh most of the work was already done by me a few months ago
fleet dome
It's always going to go like this with the level of participation in the ports
so next time, we'll be in exactly the same situation π
drifting finch
so next time, we'll be in exactly the same situation π
Then start with it split
merry falcon
How much of a breaking change for modder is 1.21.5 right now?
How much extra work will the split in Neo add?
gleaming pendant
fleet dome
How much of a breaking change for modder is 1.21.5 right now?
How much extra wo...
0
The split is not visible to modders
merry falcon
All internal stuff?
gleaming pendant
the split in Neo adds 0.1% of work (just we need to change a few APIs that are mixing client and common code)
drifting finch
The split produces the same artifacts externally
fleet dome
This is about forcing us to consider the split sources
gleaming pendant
the biggest issue with the split is that all client files get moved to a different folder
gleaming pendant
Then start with it split
we can't because of rebasing
drifting finch
we can't because of rebasing
Arg
gleaming pendant
the split has to be a separate commit that is added after the port
drifting finch
You are right....
fleet dome
Well okay, if you're renaming stuff package-wise then modders may have to account for package moves, yes
drifting finch
I hate this
merry falcon
With the minimal work exposed to modders, Iβm fine with a split starting for 1.21.5
gleaming pendant
I think the only move was ModelData to move it out of client
fleet dome
but there's no toolchain changes in moddev, for example
drifting finch
I don't want to make such a massive internal infrastructural change in a handfull of days
Because it can not get properly reviewed
And processed
On the other hand....
Does this need to happen before a release....
Maybe
merry falcon
Neo will be beta for 1.21.5
drifting finch
Maybe not
fleet dome
+536 β335 is honestly not that much
drifting finch
` +536 β335 ` is honestly not that much
Codewise it is small yeah
But infrastructurally and process wise it is much bigger.
gleaming pendant
` +536 β335 ` is honestly not that much
well that doesn't include all the moves π
that's the bigger issue
drifting finch
And that
gleaming pendant
the patches are indeed slim
fleet dome
But infrastructurally and process wise it is much bigger.
Is it though?
drifting finch
It is
drifting finch
It is a massive change as to how we have worked for over 15 years
fleet dome
We already know upstream is split
drifting finch
So yes
fleet dome
and have tried to design APIs with that in mind
drifting finch
Whether upstream or fabric or somebody else has this design is an indicator
fleet dome
so the risk of suddenly being in a situation where Mojang starts mixing client and common code
is slim
drifting finch
It is still and will always a massive shift in how we operate and work
fleet dome
and the risk mitigation is to just merge the shit together again
drifting finch
We need to do it
fleet dome
I mean tech, your stuff compiled, didn't it?
So we couldn't have been too far off from a working split NF
gleaming pendant
yes it compiled; I did some testing but more testing is appreciated
merry falcon
It is still and will always a massive shift in how we operate and work
Just put client code in client package and non-client code elsewhere? We shouldβve been doing that all along
haughty spade
is there anything that makes this change urgent?
drifting finch
Just put client code in client package and non-client code elsewhere? We shouldβ...
Yes, totally argee
fleet dome
Since the only work factor I see beside upstream suddenly throwing a wrench in it by unsplitting is that we need to ensure our APIs actually work in split sources
haughty spade
not just interesting or wanted, but actually blocking future improvements
gleaming pendant
not just interesting or wanted, but actually blocking future improvements
yes
drifting finch
But that does not change the fact that the change is not urgent (it has worked for the last few years, and it will likely work for at least another couple of months, if not years)
fleet dome
not just interesting or wanted, but actually blocking future improvements
Well it's about having clean history and moving the stuff such that modder-facing classes don't move around
gleaming pendant
we cannot offer a split workspace to moddevs if neoforge itself is not split
because it means that the patched MC sources are not properly split anymore, because of NeoForge's patches
fleet dome
well yes, that's the subsequent change that can happen later but requires this move
gleaming pendant
so the idea would be we split NeoForge now in a way that is not really visible to external users then figure out how the external users want to benefit from that later
merry falcon
Yes, totally argee
I would argue the workflow of putting client stuff in client place is a workflow mindset we should have been thinking about all this time and only breaking from that for legacy Neo things in current codebase. Splitting would bring the codebase in line of how we all operate within our own mods and what mindset we try to do in Neo
drifting finch
well yes, that's the subsequent change that can happen *later* but requires this...
Given however that we have 0 tooling to do so in place right now, it is not a direct reuqirement to merge it now
gleaming pendant
what tooling is required?
drifting finch
I would argue the workflow of putting client stuff in client place is a workflow...
Again true, but is currently not in place
fleet dome
well you need to do it now to develop the tooling later
drifting finch
well you need to do it now to develop the tooling later
That is something I am willing to debate and I am pretty sure can be debated
oblique tusk
is this being pushed to be done before 1.21.5 releases? why not after?
fleet dome
to clarify, Orion and me are talking about the tooling to later provide split artifacts for moddev downstream
drifting finch
is this being pushed to be done before 1.21.5 releases? why not after?
I would argue after
gleaming pendant
is this being pushed to be done before 1.21.5 releases? why not after?
when else can we do it?
haughty spade
is this being pushed to be done before 1.21.5 releases? why not after?
that is my question
gleaming pendant
ALL client files are moved to a different directory
drifting finch
I would do it after in a proper pr
oblique tusk
we do have a breaking changes window for a reason
gleaming pendant
it's a shitshow for PRs tbh
fleet dome
the dirctory move isn't actually breaking
oblique tusk
and a dedicated pull request and review workflow
fleet dome
but it's a nightmare for PRs
drifting finch
It is a nightmare for existing PRs
fleet dome
you can rebase and manually conflict resolve all PRs once you merge this if you had any PRs prior
fleet dome
so honestly doing this even a day or two after releasing the first 1.21.5 version will cause a ton of unnecessary work
oblique tusk
at the moment, since we have the porting branch in the main repo now, what stops us from just having the PR made against the porting branch and reviewing that? (and when 1.21.5 releases, retargeting to 1.21.x)
fleet dome
I still don't understand what the risk is for such a mechanical move?
fleet dome
at the moment, since we have the porting branch in the main repo now, what stops...
That is effectively what Tech is proposing
gleaming pendant
(the porting branch won't get further rebases BTW, it will become the 1.21.x branch as is later)
(precisely so we have a few days to land larger refactors that should not be part of the squashed port commit)
oblique tusk
my mindset is trying to separate things that must be done during a port and gets collated into the single uber-port-commit
vs what can be done as a PR that's merged in the minutes after the porting PR, so we have record
fleet dome
my mindset is trying to separate things that *must* be done during a port and ge...
yeah this is not proposed to be part of the uber port commit since we're already past that point
drifting finch
That is effectively what Tech is proposing
That is something I missed
From the way he frased it I understood it differently
merry falcon
That is effectively what Tech is proposing
Dew it
fleet dome
it's proposed to be one of the first commits past that to make PRs for 1.21.5 be based on that
fleet dome
That's how I understood it
oblique tusk
then I see no problem if so, since we can then review it properly
fleet dome
I mean the current thing is a PR too
drifting finch
But that removes the 3 day deadline
floral mesa
It still needs to be the first PR merged after the main port to avoid a bunch of manual labor
drifting finch
It still needs to be the first PR merged after the main port to avoid a bunch of...
That is something I can understand but disagree with
gleaming pendant
it can be merged before 3 days since there won't be further squashes but yes
fleet dome
I think you are overstating the impact Orion
drifting finch
I might be
merry falcon
The time is simply not long enough
What do you need for reviewing?
fleet dome
My understanding of what's being done is: MOST of our code was already split source ready
floral mesa
hot take: why do we need a day one release
drifting finch
And stating that it has to be merged in 3 days is not a good sign
merry falcon
Run techβs PR and make sure it doesnβt blow up?
floral mesa
this isn't going to be a major version for packs
haughty spade
okay so,
- I don't personally care for split sources, and I would rather not have to think about split sources in my mods
- I'm not very active as a maintainer right now so neodev-side breaks I trust your word.
- As a mod developer, I would hope that this change happening on neodev side does not break my existing project setup. I don't mind some package renames and class/field refactoring, I can adapt to those changes.
- I don't believe right now is the moment to apply those changes, in a hurry, right before 1.21.5 release on tuesday
- I would like major breaks to be ready or mostly-ready before releases, so that we can reduce as much as possible the breaking changes window (I already "hate" that it stays open for more than 1 month sometimes)
drifting finch
https://github.com/neoforged/NeoForge/pull/1920
That is a POC without moves
That was not testable in a larger setup
Not the kind of PR I am talking about
gleaming pendant
it does move files
and it should be usable in userdev, hopefully (maybe with minimal changes, I forgot if I tested that or not)
fleet dome
So what that PR should be doing as far as I understand:
- Do the plumbing to buildSrc/ no one reads anyway
- Fix any patches that were not split source ready
- Split our own code into client & main
gleaming pendant
look at the file count
fleet dome
okay so,
- I don't personally care for split sources, and I would rather not hav...
This stage doesn't change anything about userdev, and userdev will always be possible to be unsplit even after providing split artifacts for neo itself
drifting finch
and it should be usable in userdev, hopefully (maybe with minimal changes, I for...
So your PR there already moves all the files
Why is the diff count so small
Oog
Ffs
gleaming pendant
I guess a moved file counts as a diff of 0 π
fleet dome
Why is the diff count so small
Because the moves dont change anything about the files
merry falcon
lol
fleet dome
@haughty spade An unsplit userdev is just a client userdev, as far as I understand it π
drifting finch
I am going to look at it later today
I will let you guys know here if I am okey with the three day timeline then
Eating dinner first
fleet dome
I was always surprised by how little tech actually had to change about the build plumbing to get this working at all lol
drifting finch
Yeah
fleet dome
I suppose that's @OnlyIn heuristics for ya
haughty spade
this isn't going to be a major version for packs
I hate this frame of mind, the whole "it won't be popular so why care". we don't know that there will be a 1.22 in june, or december, or ever. for all we know it will be drop after drop after drop, and never have a stable version ever again
drifting finch
Yep
fleet dome
WAKE ME WHEN I CAN RIDE GHASTS
haughty spade
whatever version modpacks will use is whatever version people port mods to
floral mesa
Mods are never all ported to every minor version
haughty spade
and we shouldn't be the ones deciding this
floral mesa
and that seems guaranteed to happen while mods are developed by hobbyists with varying levels of free time
haughty spade
there are reasons to delay a port that is otherwise ready so that breaking changes can be incorporated
but honestly? split sources are not one, IMO
fleet dome
Hm, I disagree with that
merry falcon
Itβs not delaying giga
fleet dome
Not that we're delaying π
haughty spade
they are not required at all for anything other than some convenience. so while nice, I would disagree with delaying a release so that split sources can be merging
merry falcon
The port happens but the pr should be merged quickly after if we can get it reviewed properly in time
haughty spade
hot take: why do we need a day one release
that was in response to this
sorry for not being specific enough
fleet dome
@glass wagon Can we maybe remove mcp/client/Start.java from the "inject" fileset we publish in Neoform? Are you really using that outside of the NeoForm dev environment?
fleet dome
NF does, I thought NG does too. But I think it doesn't even work properly outside of NeoForm dev-env now
since it uses a special asset.json that nothing else produces
Funnily it also breaks in NeoForm for me since it references client-classes π
But gets injected into the server-dist too
Hence Tech having to patch it out here: https://github.com/neoforged/NeoForge/blob/4733b01e0f541829a3ef6dcf6969d1e0189511a4/patches/mcp/client/Start.java.patch
haughty spade
anyhow to rephrase: I think it's a good change so long as it doesn't make modder's lives harder (which seems to be the intention so that's fine),
I just don't think we need to rush this.
if it is ready by monday evening, then I would say go ahead
if it isn't quite ready yet, I would say release 1.21.5 branch as soon as it's ready, and in the announcement post, warn people of this upcoming change so they don't all rush to release something that will break in 24 hours
haughty spade
because we can start porting and testing and bugfixing
and by we I mean the community at large
merry falcon
People can start porting work to work on Mc changes first
floral mesa
I guess
drifting finch
Because that is what we do?
haughty spade
everyone that doesn't want to use unofficial builds
drifting finch
We release several times a week
fleet dome
anyhow to rephrase: I think it's a good change so long as it doesn't make modder...
It's less about modders releasing something, it's about the PRs being rebased on 1.21.5
proven compass
does this reduce the size of the server jar
floral mesa
Because that is what we do?
I meant more for this special case, that it seemed a bit wasteful to release a version knowing a planned major breaking change will follow within 24 hours. Usually we don't have a breaking change planned immediately after the initial release
proven compass
ur including client classes in the server jar even when u split?
fleet dome
almost no one publishes split jars for mods
drifting finch
I meant more for this special case, that it seemed a bit wasteful to release a v...
To users it is not a breaking change
proven compass
neo has an actual server jar tho
floral mesa
fair
gleaming pendant
the question is really whether we can get it reviewed on time
fleet dome
Well except for some of the moved classes, which TBH we could just move outside of the PR
moved not in file-system sense
in package sense
drifting finch
neo has an actual server jar tho
A patches server jar is the client
fleet dome
let me actually chekc what he is moving package-wise
merry falcon
the question is really whether we can get it reviewed on time
This. Letβs just wait till release date is closer before discussing it further if it has to be delayed
drifting finch
the question is really whether we can get it reviewed on time
I hope so. But I can get you that information later today.
fleet dome
Oh. let me guess, that's actually really shit to see in the GH UI π
haughty spade
A patches server jar is the client
minus OnlyIn(CLIENT) stuff, no?
drifting finch
minus OnlyIn(CLIENT) stuff, no?
Nope
haughty spade
vanilla-wise that is
fleet dome
wellllllllllllllllllll
drifting finch
It is a full client jar
drifting finch
The binary patches for the server jar patch all client classes back in
haughty spade
we patch-in all the client classes into the server, and then refuse to load them??
drifting finch
So that the side stripper can check for the annotation at runtime
fleet dome
you dont have the libs
haughty spade
that seems ugly somehow :P
fleet dome
and we're going to stop doing that at some point if we resume work on the manifest-based splitting
drifting finch
Yeah
We want to eventually
Track the manifest of the jar what classes of the client we can and can't koad
haughty spade
our server patch should only include and apply classes that are not OnlyIn(CLIENT)
merry falcon
how much faster will it make Neo? 10 milliseconds?
drifting finch
our server patch should only include and apply classes that are **not** OnlyIn(C...
You think so
But no
Because the only way to get a nice error
haughty spade
it's not about fast, it's about not including hundreds of mojang classes in the patch
drifting finch
Is to have the underlying class loader load the class
And then have the transformer see the annotation and throw
Which is suboptimal...... For the lack of a better word
Sharte has a good idea to use the manifest to store the data
A d then throw based of that
Which would help a lot here
haughty spade
yeah that would be nice
or even a separate file
doesn't need to bloat the manifest
drifting finch
Meh
I liked his solution
It was neat and tidy and matched existing jvm infra
So I don't care
gleaming pendant
or even a separate file
classic bikeshed example π
fleet dome
doesn't need to bloat the manifest
The manifest is intended for stuff like this, imho
It specifically has per-file attributes
and you get it as the first entry of the jar always
which means if you do a ZipInputStream based splitting, you have it avaiable immediately
fleet dome
(Although to be honest, we should probably not do that if we can avoid it, since local file headers in ZIP files are an attack vector)
The manifest parser is optimized for this
don't forget that the official Mojang manifest has digests for every file in the jar
fleet dome
Well... that's how jar signing works
fleet dome
Me too π
haughty spade
so every time someone reminds me, I go "ew"
drifting finch
That concept still baffles me
It is a step above 0
Which is better then nothing at all
fleet dome
I'd not be surprised if they remove it
drifting finch
Lets not waste time on this
merry falcon
Doesnβt forge and Neo try to do jar signing or did we rip it out?
fleet dome
it doesn't do anything
floral mesa
don't forget that the official Mojang manifest has digests *for every file in t...
which consumes a nontrivial amount of heap space, last time I checked
fleet dome
we drop it
FART does, IIRC. Or rather, there are multiple places that drop it π
But *ART has code for doing it since that's a logical place to do it. When you remap the .class files, their digest is obviously no longer the same
haughty spade
IMO jar signing is done wrongly
there should be a file at the end of the zip, that includes entries for every previous file in the zip, including the manifest
gleaming pendant
I think the discussion has diverged for long enough π
haughty spade
and signs the whole list of files, such that additional files are also tested for :p
yeah XD
drifting finch
@gleaming pendant I kind of like the PR, and we can get it reviewed.
However I am not sure I like the concept of NeoForgeProxy.
I wonder if it is possible to handle this better through a part of the FML Mod Construction logic
I was thinking about how to instantiate this better
We might want to consider some other process that other mods can eventually also implement
It is reviewable
in 3 days though
Again I just don't know if the internal setup of the Proxy is something I like
gleaming pendant
yeah the Proxy is simply my go-to solution in these cases but it might not be the best way
drifting finch
Yeah mine too
gleaming pendant
if we do go for a proxy as the standard solution (also in mods) we should ideally offer a helper to instantiate it automatically
drifting finch
And that made me realise
That if we are all doing this
It might not be the best architectural solution to go with
drifting finch
if we do go for a proxy as the standard solution (also in mods) we should ideall...
That is kind of my point
I do that for all kinds of things
My API has this logic
My PROXY has this logic
It is infuriating
But to solve this properly
We really kind of need some form of DI that is more substantial then what we have now in FML
gleaming pendant
API is a bit different; I suppose you can't see the internals from the API so you have to reflect them?
drifting finch
And take DI here in the softest sense
drifting finch
API is a bit different; I suppose you can't see the internals from the API so yo...
Yep, sometimes I use a Holder class like you did with the Proxy here
Sometimes I use reflection
What ever suits the situation the best
I would really like to offer modders the ability to say:
I need IModProxy injected into the constructor
And the platform figures out whether it should be the client instance or the server instance
That pattern
It happens so much through out all of our code bases
And we never tackled the annoyance with it
gleaming pendant
well it is once per mod and a few (ugly) lines
drifting finch
Sure
But if we now start using it ourselves
Might as well at least think about a better mechanism
And whether it is worth implementing
gleaming pendant
yes
drifting finch
(It should not stop this PR though)
As the Proxy is declared internal
But you know
Food for thought
gleaming pendant
your API-needs-to-talk-to-impl example is a bit different though since the selection criterion is different
drifting finch
Ah no you have it backwards
My API never has this problem
I have two API surface implementations
One for client
One for server s
So my APIs behavior is the same as the proxy
Just another instance
gleaming pendant
ah it's simply not called proxy but it uses the same idea of having one impl per dist?
drifting finch
Correct
I don't need it everywhere
But sometimes I do
Like we do all here with the proxy
In most cases we don't need it
But sometimes for some method constructs we need it
gleaming pendant
what I had in mind was more of an annotation, e.g.
@SideImplementation(value = NeoForgeCommonProxy.class, dist = Dist.CLIENT)
public class NeoForgeClientProxy extends NeoForgeCommonProxy {
and then
ModLoader.findImplementation(NeoForgeCommonProxy.class);
or whatever
drifting finch
That is exactly what I was thinking too
gleaming pendant
I just worry that we create something way more complicated than the reflection we are trying to replace
drifting finch
Maybe
I had the same thought
But having something like this plumbed into the modconstructor logic
Would be soooo nice
Potentially with a global scope
Where you can just request the API etc of other mods through the mod constructor
That would result in a very nice and flexible api
That ties mods together
And allows them to easier work with and for each other
fleet dome
IDK I never liked the idea of calling it a proxy
fleet dome
what I had in mind was more of an annotation, e.g.
```java
@SideImplementation(v...
Please don't
π
I really like that we got away from the annotation magic for this
And essentially moved to split mod entrypoint
Unless we do go with DI
like actual DI
drifting finch
Unless we *do* go with DI
I really would like to
But java is shit for this
It really is
gleaming pendant
And essentially moved to split mod entrypoint
yeah in many cases the proxy instance (or whatever you want to call it) can be set from the mod ctor
but I prefer to have it final
fleet dome
well, final on what though
gleaming pendant
final
fleet dome
static final? final?
drifting finch
mine are not final
They are static
Because I need to construct and set the API and PROXY from the mod constructor
gleaming pendant
I like it static and final π
drifting finch
Mee tooo
But alas
It should not block the PR
But we should discuss our infrastructure in this area
weak lichenBOT
Created Gist at the request of @merry falcon: https://gist.github.com/NeoCamelot/a23102dbd92d8128172ef650ea1f6d70
oblique tusk
well, first off, what were you doing immediately before it crashed
fleet dome
No that's a red herring
at TRANSFORMER/[email protected]/net.minecraft.client.renderer.MultiBufferSource$BufferSource.endBatch(MultiBufferSource.java:87) ~[%23204!/:?] {re:classloading,pl:runtimedistcleaner:A}
at TRANSFORMER/[email protected]/net.minecraft.client.renderer.MultiBufferSource$BufferSource.getBuffer(MultiBufferSource.java:57) ~[%23204!/:?] {re:classloading,pl:runtimedistcleaner:A}
at TRANSFORMER/[email protected]/net.minecraft.client.renderer.entity.layers.LivingEntityEmissiveLayer.render(LivingEntityEmissiveLayer.java:47) ~[%23204!/:?] {re:classloading,pl:runtimedistcleaner:A}
It's flushing a previous draw when the emissive layer is trying to get its buffer
limpid crypt
oh ok
oblique tusk
I still need you to answer the question, Lolo
limpid crypt
full log file
fleet dome
Warden
limpid crypt
well, first off, what were you doing immediately before it crashed
nothing
merry falcon
Well consider a warden was around, you were doing something
oblique tusk
I don't think summoning a Warden into the world counts as doing nothing
oblique tusk
I can literally see the log saying that you summoned a Warden in the milliseconds immediately before you crashed
so please, tell me why that counts as "nothing"
merry falcon
Answer carefully lolo lol
limpid crypt
Yes, it was to reproduce the bug
fleet dome
... which bug
this bug?
You know, your report would be more useful if you just said
"I ran /summon <blahfasel> and it just crashed like this:"
oblique tusk
so you were fully aware you were doing something
...yet you said you were doing nothing before it crashed?
please tell me how that makes sense
drifting finch
Lolo, I warned you earlier today that you should only report stuff you can reproduce and with the steps
And not here
But on github
I also said that was your last warning
Now you are in luck tonight
As I am in a decent enough mood
So you get another shot
But if I see bullshit reporting like this again I will mute you here and on the github org
Do we understand each other?
drifting finch
Good
limpid crypt
When I sent the crash report, I didn't know what caused the bug, but I saw after sending the log file that it was caused by the Wardens
drifting finch
When I sent the crash report, I didn't know what caused the bug, but I saw after...
Yeah, so what did you do wrong?
You posted before thinking
So think and read first
Before posting next time
limpid crypt
I'll report it on Github
drifting finch
@oblique tusk We might want to make an Uber ticket that holds all other issues found before release
Like a TODO, do this before 1.21.5 release kind of thing
Where his tickets are sub-tickets to handle
So we have some kind of overview of it all
Do you concur?
fleet dome
for (int i = 0; i < 12; i++) {
GpuTexture gputexture = RenderSystem.getShaderTexture(i);
if (gputexture != null) {
renderpass.bindSampler("Sampler" + i, gputexture);
}
}
Jeez what the hell mojang π
drifting finch
tf am I looking at?
floral mesa
This is not new
drifting finch
Do they not know which textures are bound in the shader......
fleet dome
They do that on every draw
floral mesa
They have been doing this since 1.17
drifting finch
This is not new
No it is not...... But i thought with the pipeline rebuild they could track this.....