New Lighting Pipeline

[wicked lodge]

Replace the experimental light pipeline by a new "enhanced AO" pipeline derived from Indigo's (the reference implementation of the Fabric Renderer API).

See https://github.com/neoforged/NeoForge/pull/2078

opening this thread for discussion already 😛

and my first question is: wtf is this?

[fallow ledge]

wdym?

it's the statically applied diffuse lighting

based on presumed "sun direction"

[native mulch]

The vanilla shade function only works for the 6 axis-aligned directions, the extended Neo one computes a shade in any direction by interpolating them in some fashion, I believe

[fallow ledge]

well a level should presumably override that to statically compute the diffuse light from the sun based on the actual normal,
but if the level doesn't, it tries to fall back to the vanilla impl

I'd assume it's the combination of DIFFUSE_LIGHT_0 / DIFFUSE_LIGHT_1? 🤔

[wicked lodge]

do people actually use this method?

[fallow ledge]

I thought your lighter would use it

for non-axis aligned quads

[wicked lodge]

it does a similar normal-based interpolation, but it interpolates more than just the shade

maybe this makes sense for flat lighting though (which I haven't touched)

[fallow ledge]

I am also confused by this I'd have expected the nether to have different lighting but apparentl ynot

Oh that's quite possible

[cinder field]

in vanilla it does, the nether has a different set of multipliers per direction

controlled via the DimensionSpecialEffects#constantAmbientLight boolean

[fierce tartan]

Does this pipeline fix the issues for blocks that are larger in size than one block?

[wicked lodge]

no

well, the best I can do is clamp vertex coordinates to [0,1]

what do you have in mind?

[atomic comet]

The Neo extension also respects that flag

[wicked lodge]

it does it in a weird way though

the multiplication by the normal y is very suspicious

[atomic comet]

The calculation makes sense for the most part if you compare it to the vanilla ClientLevel#getShade() implementation. The only thing that looks odd to me is the use of squaring instead of Math.abs() for the normal values on the second line in QuadLighter.calculateShade()

[fallow ledge]

lol i completely read past that as "hey it just applies some ambient light"

that's kinda shitty 😄

[frozen loom]

The functionality of the method is useful and Indigo replicates it but whether anyone actually wants to override it is another question

Its current implementation is not correct though

[plucky magnet]

I wonder what copilot thinks about your code tech /hj

[fallow ledge]

do it 😄

genuinely curios

[vapid bough]

🙈 ->

[plucky magnet]

sure

https://github.com/neoforged/NeoForge/pull/2078#pullrequestreview-2727733371 mhh yes

much review

I agree with its very extensive review

seems like it doesn't do much more than what an IDE's static analyser would do

weird code branches, suggested parantheses and typos

[fallow ledge]

it also cant see the vanilla code

[wicked lodge]

Lol ok, useless "review"

[undone chasm]

Well at least it summarized the changes per file heh

[deft gorge]

curios, you say? /j

[fierce tartan]

Idk fully, I think @native mulch explained to me at some point what needs to be adjusted to make it work

[native mulch]

Clamping + computing diffuse shade based on the quad's actual normal is enough to make the digital miner look correct, afaik

[wicked lodge]

turns out that these 3 TODOs are related

to fix the seams, the mean light formula has to be changed, but that introduces issues with emissive blocks which require further thinking

[atomic comet]

If you need more weird test cases for emissivity and light emission handling: you can make any framed block emissive and/or emit light with Glow Paste and Glowstone Dust respectively. The light emission specifically will force AO to stay enabled if it comes from Glowstone Dust and not a light-emitting camo

[wicked lodge]

ah that's great thanks, the sculk sensor is really annoying because it doesn't remain turned on

[covert plover]

C&B Blocks are also a really good test

AS you can set them to scaled light emission with glowstone/ink sacks

And then they depend on how many bits are in there that emit light

[fallow ledge]

remember the thing we had with the ME drive, I'll see how far I can get this working again, but that was extremely broken in the experimental pipeline

[wicked lodge]

yes

actually that would be a good test case now that AE2 is being ported to 1.21.5

[covert plover]

I don't really like this API: https://github.com/neoforged/NeoForge/pull/2078/files#diff-fd840cf23fa63754f09a96e8c0eea17f3d4060a651578422f27099fc10d55075R203

Is there any risk that it is not called properly?

[wicked lodge]

there is a risk yes; quite low though, and the EnhancedAoRenderStorage will throw which means that the mistake will be caught easily

[covert plover]

I mean in particularly related to custom render mods

[wicked lodge]

I think that they would sidestep a lot of the vanilla pipeline

they might use the AO pipeline directly though

[covert plover]

I am worried about that last case

Not enough to care really

[wicked lodge]

we'll see I guess; sodium has its own (rather similar) pipeline

[covert plover]

Just something to think about when taking a shower / sitting on the loo / where ever you get your next good idea

What?

Don't tell me neither of you two have had the need to run out of the shower to write down a very nice code solution to a problem that had been plagueing you for several days..........

At one point I was considering leaving paper and pencil in the bathroom just incase

[wicked lodge]

doesn't usually happen to me 😅

[atomic comet]

I've surprisingly only had that issue when I'm trying to sleep because that's, annoyingly enough, the time where my brain is the hardest to get to shut up

[covert plover]

Count yourself lucky

[covert plover]

Don't have that issue, can sleep anywhere, even while walking

Have done so many times

Does not end well with lightpoles

Don't ask how I know that

[native mulch]

Absolutely

Tens of times

[covert plover]

https://github.com/neoforged/NeoForge/pull/2078/files#diff-cff89e6c22a22e2ed618c40cece9543b307fae422486f70dcf257075e1095557R490 You might want to consider documenting this method...........

It took me reading it three times to understand that it was retuning a conditionally packed normal

[fallow ledge]

conditionally?

[covert plover]

Oh right not conditionally

What am I reading tonight......

Snuffed to much paint

[fallow ledge]

Looks just like a packed normal. Is that the packed 8_8_8_8 normal format mc uses?

[wicked lodge]

yeah I really need to document that one, and use it in fillNormal just below

[covert plover]

https://github.com/neoforged/NeoForge/pull/2078/files#diff-cff89e6c22a22e2ed618c40cece9543b307fae422486f70dcf257075e1095557R490 Does this not need to be scheduled?

[wicked lodge]

yes, except that the last component is unused 😄

[covert plover]

Was that never scheduled?.....!!!??

[wicked lodge]

computeQuadNormal is used to light irregular faces, in case a vertex has a 0 normal

[covert plover]

Ugly..... https://github.com/neoforged/NeoForge/pull/2078/files#diff-cff89e6c22a22e2ed618c40cece9543b307fae422486f70dcf257075e1095557R490

[wicked lodge]

(by then computing the quad normal)

might be unnecessary, idk

people might not fill in their normals correctly?

[wicked lodge]

the manual normal computation?

[covert plover]

No

Wrong link

Hold on

https://github.com/neoforged/NeoForge/pull/2078/files#diff-9e2973af2b0ae416e601fbab29d70cf3026d0bf23d04bc6a3c05ffaa10e13aeeR11

The AoCalculatedFace

Pure package private fields.....

I know why you did it

But it reads so....... like C

[wicked lodge]

ah right; waiting for valhalla

[native mulch]

this is a very C-like part of the code

[wicked lodge]

I'm pretty sure Grondag was a C programmer given some of the code he wrote in Indigo; but then that's also how you can get good performance out of Java sometimes

I really don't want to allocate any additional memory compared to vanilla's pipeline; certainly not up to 24 such AoCalculatedFaces per block

[fallow ledge]

No you have to do this

so even if you're not a C coder

this is kinda what you gotta do in Java

In C it'd be cleaner...

Given any fixed size arrays or nested stucts (i.e. Vectors) would be inlined into the parent structure rather than heap allocated

the C version of this is easier to read 😄

[covert plover]

Yeah you have to do this

As I said I know why you did it

But it is still Ugly

Ugly but necessary

[fallow ledge]

I am surprised it doesn't go further and just uses one fat large float[] 😄

[covert plover]

This is probably faster when the JIT starts inlining

[fallow ledge]

I'd not be so sure, especially with the fat object headers

[covert plover]

Hmm maybe

[fallow ledge]

if it's one array, it's guaranteed to be contiguous and should (tm) be better for cache? I'd hope

[wicked lodge]

float[] is one more indirection

[fallow ledge]

no i meant instead of the entire class

but nevermind, I don't know that code well enough to say if that current AoCalculatedFace is used standalone or in an array again

[wicked lodge]

well it has both ints and floats

[fallow ledge]

well so has int[] vertices 😄

[wicked lodge]

heh true

could be done, they're not used in that many places

however it would make the code a lot less readable

FramedBlocks emissive is actually not helpful 😛

because it also contributes to the light level of adjacent blocks

I need something similar to the sculk sensor

[atomic comet]

You mean the light emission. If you want the vanilla wannabe emissivity, then using a magma block as the camo should work, I'm pretty sure I got my blocks to properly copy over that behavior

[fallow ledge]

yes it wasn't a super serious suggestion but in java without valhalla it's probably the best one could do

[wicked lodge]

yeah that does the trick, thanks

(this is a framed cube)

could probably just have used a magma block 😛

[atomic comet]

For the full block at least, yeah. My suggestion earlier was primarily targeted at odd shapes and solid vs. non-solid blocks (though, in hindsight, the latter won't work for testing emissivity as you found out since my emissivity doesn't have the fake light emission)

[wicked lodge]

I can only hope that complex emissive geometry won't look bad

[atomic comet]

Btw, with respect to the ME Drive: you could just throw the model at a vanilla block in NeoDev, the issue is only visible in empty drive slots anyway, so you don't need the fancy dynamic model to make it visible

[wicked lodge]

yes true

btw I'm not too happy yet, this corner looks weird

somehow the vertical quad is brighter

[fallow ledge]

Yes correct, it was actually the static JSON model that had the problem

[frozen loom]

Did you apply the edge 1 fix?

[wicked lodge]

Hmm no

There is a bug in vanilla?

[frozen loom]

Yes

[wicked lodge]

Wtf 😅

Ok I'll have a look tomorrow

[frozen loom]

Indeed it is missing
https://github.com/neoforged/NeoForge/pull/2078/files#diff-76c557b2145b7608aee6081448475de8f75bc3518df1ea95b600261bd5eb88ccR146

I added those Indigo comments above where Indigo deviated from vanilla

[wicked lodge]

Ah indeed, it should use side 1 or 2

[fallow ledge]

"the edge 1 fix"

jesus

[wicked lodge]

Was this ever reported to mojang?

[frozen loom]

I'm sure there are plenty of reports about smooth lighting bugs on the tracker

[wicked lodge]

Sounds like such a low effort fix

Hi @misty terrace is this something you are at all interested in? Some corners in the AO computation are using the wrong edges, it's a really simple fix. Maybe AO is not a focus area atm

[frozen loom]

It is, and so are 90% of the other fixes, but they still haven't been made so I don't know what the reason for that is

[atomic comet]

I'm gonna mention this here since it's somewhat related: while looking for cleanup opportunities in my model implementations last night, I stumbled over the problem of per-part AO again and decided to try implementing it. Turns out it's fairly simple to do and with a minor inefficiency it would even be doable without any breaking changes: https://github.com/XFactHD/NeoForge/commit/b926affa410732fcaec8ccc2b232c52de2725e29

[wicked lodge]

Wdym by per-part AO?

Quads are not granular enough? 😛

[atomic comet]

It's too granular in certain cases (i.e. unnecessarily requires deep-copying a part when shallow-copying it with just a changed AO flag would work just fine) and also just plain doesn't work when the first part of a model disables AO but another part wants AO

[wicked lodge]

How does it not work? Does the quad not get final authority?

I know it's documented, but choosing whether AO is enabled or not at 2 levels is already a mess. Making it 3 levels sounds even worse. 😅

[atomic comet]

The quad can only disable AO, it cannot re-enable it. My patch also doesn't make it three levels (there is no per-model AO anymore), it just makes the first level actually work properly.

[wicked lodge]

Isn't there a TriState on the quad?

No it's on the model

Pff

I actually prefer the FRAPI way with a boolean on the model and a TriState on the quad hehe

[atomic comet]

Even if the quad had a tristate for it, it wouldn't work due to the part (or previously the model) deciding whether the AO machinery is even set up at all

[frozen loom]

It is always set up in FRAPI impls so a TriState works fine there

[atomic comet]

Well, yeah, but we don't have that luxury with the extended vanilla implementation (unless we do something very similar to what the linked commit does, at which point we might as well do it properly and respect every part's AO setting)

[wicked lodge]

Yeah the current AO hooks are a bit suboptimal

[atomic comet]

Also, even if we made the quad have final authority with a tristate, it doesn't change the fact that requiring a deep-copy of the part (including shallow copying every single quad) is stupid

[wicked lodge]

Yeah well I don't enjoy splitting quads into 256 parts just because each part has a different combination of properties 😛

But I did think about moving some quad properties back into the part (particularly the LightingMode I am adding)

[atomic comet]

Nothing forces you to, you can achieve that with what we have right now

[wicked lodge]

Hmm?

I mean that we might even consider removing per-quad AO and turn it into a part property 😄

[atomic comet]

Ah, then I misunderstood what you meant

But, to avoid any confusion: AO is already a part property in vanilla, they moved it there from the model. We just extend that to allow force-enabling AO (i.e. bypassing the fact that vanilla normally force-disables AO for light-emitting blocks)

[wicked lodge]

Ah yes right

I have to look at your commit cause now I'm confused

[atomic comet]

Basically, previously vanilla checked BakedModel#useAmbientOcclusion() to decide whether the AO or non-AO path is used and now they check partList.getFirst().useAmbientOcclusion() which means that the first part has complete authority over the AO and the other part's AO flags are ignored. The way we augment that AO check with the tristate is equivalent between these two variants

[wicked lodge]

Ahh right

[wicked lodge]

wait vanilla AO always uses the brightness and lightmap from the edge 0 if both edges are opaque?

that's incredibly stupid actually 😄

[frozen loom]

Yes that's the bug

[wicked lodge]

I thought it was more subtle, but no they just always use 0

much better now with the fix applied

now I still need to look at the center light sampling, and maybe the clamping fix

@frozen loom what is the problem here? this is mentioned as one of the problematic cases of the vanilla center light formula

[wicked lodge]

yup

[frozen loom]

Removing the corners makes it more apparent because that reduces the amount of AO on the sides

[wicked lodge]

and this can create seams

[frozen loom]

Oh I didn't even find that

I wonder if the Sodium formula actually fixes that

[wicked lodge]

I think it should be designed such that the inside light of the current block matches the edge light of an adjacent block

otherwise the blending will always be inconsistent for the same corner

you can really see vanilla's logic as "force sample outside if the target block is not solidRender"

so why does it even break with white concrete? is that not solid render?

[royal gate]

Let our battle's begin!

[frozen loom]

Actually I think I remember that case from testing I did years ago

The issue is that the adjacent block cannot see the corner because it is obstructed, and that will inherently produce discontinuities

[wicked lodge]

the problem appears because quads that are not on the edge of the face are getting a mix of the inner and outer light values, I suppose

in this case exactly 50/50

and in this case I suppose that the outer light is a perfect 0?

[wicked lodge]

Sodium's formula fixes this case but I don't know why

ok I think the problem was with the inner light actually

I don't know

this is the code (vanilla)

blockstate9 should be white concrete which is solid render

I wonder if simply if (sampleOutside) { would not be better

[frozen loom]

That fixes the "sensor above slabs" case but not the "slab below ceiling being too dark" case

I don't think that would fix the discontinuity anyway

[wicked lodge]

indeed it doesn't

but why does it not

ok I know why

[frozen loom]

This

[wicked lodge]

exactly

with Sodium's formula

[frozen loom]

I think the center lightmap formula does not produce this

It is purely the outcome of inset face blending

Actually the vanilla formula might cause a discontinuity in that case even without inset face blending, but I'm pretty sure it would look completely different and might be too subtle to notice

[wicked lodge]

the underlying problem is that the outer light is discontinuous

which the blending makes visible

maybe the fix is to not blend the outer face if the target block is solid?

that will cause other kinds of discontinuities though

[frozen loom]

That's right

[frozen loom]

Yes, because then only the center slab will use blending

[wicked lodge]

Might be possible to keep track of this extra info in AoFaceData

and blend it smoothly

[frozen loom]

It doesn't sound like a very clean solution

I think a proper solution would require really deviating from vanilla, and that's what we were trying to avoid

[wicked lodge]

well, #1 goal imo is to avoid discontinuities cause they look really bad

maybe the blending formula for inset faces can be adjusted

e.g. 1/2*min(a,b) + 1/4*a + 1/4*b

(similar to how the different axis are blended for irregular faces)

[frozen loom]

They already are as much as possible, and any enhanced pipeline already has way fewer discontinuous cases than vanilla's pipeline

[wicked lodge]

"as much as possible" is zero 😛

[frozen loom]

Well, maybe, but I think trying to fix them all will waste a lot of time

I have an extra case of discontinuity for you if you want

[wicked lodge]

sure, send it 😄

[frozen loom]

I do not have a screenshot but you can replicate with the FRAPI testmod using the enhanced AO octagonal columns, positioned like this, where each [] is a column block:

up
[]
[][]
[]
down

I can get a screenshot in a few hours if you still need it

[royal gate]

Eple (original edit)!

[wicked lodge]

so 3 stacked columns + 1 next to the middle one?

[frozen loom]

Yes

[wicked lodge]

ok let's see

do I need to check out your PR for this?

[frozen loom]

No, it's always happened

Maybe my PR will change the look of the discontinuity a little bit but it should still be there

[wicked lodge]

yup

[frozen loom]

Have fun with that one

Maybe there is a good fix but I am done thinking about smooth lighting for now

[wicked lodge]

lol 😄

it's likely a similar case with the center outer light sampling

this is from the inside of the column on the right

looks like the outer face is fine

this one might be a corner obstruction problem 🤔

(the octagonal column is not considered clear)

maybe we can fix these issues now that the clear booleans are available in the blending function

[wicked lodge]

@frozen loom I think that the real bug is that octagonal is considered opaque even though it's not

that's a bug in the octagonal column

[frozen loom]

Oh, does changing that fix the issue?

[wicked lodge]

let's see

I had to look at neoforge because fabric API's workspace is really slow for me :/

even finding all implementations of a block method is bad

[frozen loom]

There is some focus script or something

Most agree there are too many modules

[wicked lodge]

ah yes I found it lol

even starting the game takes forever :/

probably "does this file exist" called 100 times for every loaded class

I don't see a seam anymore

I just added .blockVision((state, world, pos) -> false) in the registration

[frozen loom]

I feel like it should though

[wicked lodge]

blockVision is only used for some effect rendering thing and to check if a block is clear in the AO code

[royal gate]

Reticulating splines!

[frozen loom]

Yeah that's half of the clear check

I'm just not sure it makes sense to consider these blocks clear, if a mod were to actually add them

[wicked lodge]

so IMO the logic should be that if a block contains inset faces it should not block vision

that will fix all issues related to the corner not being sampled

[frozen loom]

Vanilla farmland blocks do not adhere to this

[wicked lodge]

they don't have a full shape

so their lightBlock is probably 1, which means that they are considered clear

[frozen loom]

Then I'd say that "3D" resourcepacks like to add inset faces to solid blocks

I guess those will be primarily using vanilla lighting though

[wicked lodge]

I'm not sure I want to offer a per-quad choice when using NeoForge

[frozen loom]

Even if it's per-part that would be the case

[wicked lodge]

I mean that it might not be an option that the model can choose at all

just "enable or disable enhanced pipeline"

[frozen loom]

I see

Then those cases should definitely be considered

[royal gate]

Matt Damon!

[frozen loom]

That's right

[wicked lodge]

I don't know how much thought resource pack authors give to AO

(as long as it looks reasonable)

[frozen loom]

Yes, if it's close enough it's fine, but there are cases where enhanced AO makes the blocks look completely different

[wicked lodge]

well if it goes from ridiculous to reasonable, I say it's fine

[frozen loom]

What about the infamous path lighting?

[wicked lodge]

I don't see an issue with fixing it

[frozen loom]

Alright

Perhaps there are no other significant differences past that

[native mulch]

What do you mean by "fix" it

Make it behave like vanilla or make it behave logically correctly?

[wicked lodge]

make it behave logically

[native mulch]

this will anger a segment of the community

then again nobody complained back in 1.12 when it was forced on them by default

[wicked lodge]

will it? sodium has been fixing it for a while I thought

[native mulch]

Yes, and there's a (rather popular) add-on that undoes that

[wicked lodge]

link?

[native mulch]

https://modrinth.com/mod/sodium-shadowy-path-blocks

[frozen loom]

Yeah

[royal gate]

Board game version also available!

[frozen loom]

Also allowing users to turn off enhanced AO for everything will only cause more reports for mod developers that require enhanced AO for things to look correct

[fallow ledge]

i mean that path... jeez talk about "happy accident"

[bleak solstice]

"fixing" the supposed regression to path blocks involves breaking a lot of other cases in a manner that is much worse

[fallow ledge]

i hope that "fix" implements it using a CTM style implementation since using lighting for that pronounced effect is really not right

[bleak solstice]

no, that mod just hacks in the same broken logic.

[fallow ledge]

well great

[frozen loom]

Using mixins no less

[fallow ledge]

for such a small difference in height, "real" ambient occlusion would be miniscule

[native mulch]

it used to use reflection, in a hot path

[fallow ledge]

and I say that knowing that the vanilla version "Looks" better

But I don't perceive that as "ambient occlusion"

[bleak solstice]

the issue is that no other block wants this behavior, and it often causes issues with modded content

[fallow ledge]

It's just a less uniform dirt texture lol

[bleak solstice]

special casing path blocks in particular could be done, but it is a very ugly workaround

[fallow ledge]

hence my comment about CTM

[bleak solstice]

I've been asking for years that people use CTM or something to solve the issue more robustly but there hasn't been any offer

And Mojang is not interested in doing anything about it

[wicked lodge]

well, LightingMode could be used to fix this

on fabric that is

i.e. this

[bleak solstice]

I hate to be the person who says "the users don't know better", but truly, the users are not capable of understanding how this seemingly trivial "fix" will regress the behavior of modded content in unexplained ways

And mod authors get enough issues pertaining to their models not being lit consistently or correctly

[fallow ledge]

Yes however, the path does look better when it has pronounced edges. It just shouldnt be done using lighting

[royal gate]

Rule #1: it's never my fault

[bleak solstice]

I do not like this bot.

[wicked lodge]

I have no problem supporting a hybrid VANILLA-like/ENHANCED mode currently

however it might get difficult if I fix more lighting bugs

[fallow ledge]

It's the uncanny canine AI

[wicked lodge]

any change to FullFaceCalculator will affect both VANILLA and ENHANCED

[native mulch]

Couldn't you just... run the vanilla logic for VANILLA

[wicked lodge]

that's also an option yes

but then you keep all the shitty bugs 😛

[bleak solstice]

I opted to not supporting it in Sodium since the lighting code is very difficult to maintain without allowing users to opt into broken logic

[native mulch]

Default to enhanced, special-case vanilla by default for paths so people don't complain, allow models to opt into vanilla if they really want to

Considering we already have to inject the replacement lighting logic somewhere, doing it conditionally doesn't seem like a big stretch?

[wicked lodge]

that's what I have right now

[native mulch]

The ability to force vanilla or emulated seems unnecessary outside of testing

[wicked lodge]

well it's 0 effort to maintain currently

[native mulch]

fair

[wicked lodge]

vanilla disables the pipeline entirely, so it's also good to have

btw this addon is incredibly cursed 😄

@fallow ledge you really think this looks sane? 😛

[fallow ledge]

Do not think of it as lighting

Then? yes, maybe with the exception of single blocks 😛

[wicked lodge]

this one is okay

but the single block is awful

[bleak solstice]

It is worth mentioning at least that Bedrock Edition doesn't appear to have this bug with the grass paths, and they render as if this patch was applied

I don't care much for the aesthetics argument at the end of the day, more just that I don't like the idea of special-casing certain blocks in the game

[fallow ledge]

yes but your end-users care for the aesthetics 😄

since they want nice blocks to build nice paths

they dont care whether that is achieved through borked lighting or not

[bleak solstice]

my point is that it is subjective and discussing it won't get anywhere

people can argue "but users don't like it" but that may as well be a very small minority without any numbers

[fallow ledge]

Well it is glaringly obvious though in the screenshots that "mod" posted

and I'd say paths are not that uncommon due to usage in villages (I think)

[bleak solstice]

well, yes, it is different. but the question of whether NeoForge should change things (objective) is different from whether NeoForge is making an improvement in aesthetics (subjective).

[wicked lodge]

I wonder if it really makes sense to be blending between the inside and outside light

[bleak solstice]

in this case or all cases?

[wicked lodge]

all cases

[bleak solstice]

no, not really. you can make it fail with things like stair blocks.

but a robust solution is prohibitive in both implementation complexity and runtime complexity

the blending trick generally works for the vast majority of content, and isn't too horrifically wrong even for some weird models

[bleak solstice]

if you look at the roofs of village homes for example, you'll see the interpolation across some faces of the stair blocks is wrong

this has to do with the fact that you can only assign light values to each corner, so when a quad is "divided" by an adjacent block and only partially exposed to light, you get problems

[wicked lodge]

this + no depth-based blending might actually work really well

[wicked lodge]

let's see

[bleak solstice]

you're welcome to try, but I don't think the problem is solvable unless you divide quads along the transition point and emit more geometry

essentially, you just need more light values, with a perfect solution requiring per-pixel values

(the latter of which is impossible unless you resort to path-tracing)

[royal gate]

There's <<a cat on ,my keyboard!~

[wicked lodge]

there doesn't need to be a perfect solution

my problem is that blending between the inside and outside faces seems to introduce more issues than it solves

in particular the moment you start blending with an outside face that is covered by a block, you will be screwed by corners not getting sampled if both edges are solid

the trick is to never end up in a situation where sampling the corner would have mattered

[wicked lodge]

what I want to do now is collect some "tricky" cases and put them all in a structure file that we can track in oru repo

so that one can inspect visually if changes to the lighting algorithm are sound

(and add "regression tests")

not sure if we can/want to automate this testing though

[fallow ledge]

can't you just capture the resulting vertex colors and do a bog standard unit test?

[wicked lodge]

I can but it's not a good test

[fallow ledge]

obviously you dont write the reference values manually

[wicked lodge]

you cannot really quantify that it has to "look good"

[fallow ledge]

It's a regression test

it's not about looking good 😄

you previously manually verified for the case that the reference "looks good"

and doesn't change unintentionally on a port

[frozen loom]

Even if the values are slightly different doesn't mean that the test necessarily fails, if the difference is necessary to fix something else

[fallow ledge]

since you don't need to test that linear interpolation between vertices works, you really only need to assert the vertex "color" (wich should be a grayscale value)

[wicked lodge]

yeah but the vertex colors really don't matter

[fallow ledge]

aren't the vertex colors your output?

of your AO

[wicked lodge]

yes

but again there is no ground truth

the only constraint is that it looks good

[fallow ledge]

what are you talking about 😄

[wicked lodge]

do you even know the kind of change I am making?

[fallow ledge]

the ground truth for a regression test is just the status quo

[wicked lodge]

no, we have to change the status quo

[fallow ledge]

I am talking about after you making that change

we are talking about different stuff I think

[wicked lodge]

yes, this is very frustrating

I do not want to test exact color values

[fallow ledge]

I am talking about making a regression test set once your change is adopted

[wicked lodge]

what will that help for? my main concern is not that the lighting algorithm breaks with updates

my main concern is that fixing one problematic edge case causes issues on other problematic edge cases

[fallow ledge]

yes that will only help going forward

It would only help with discovering that it did not how or if the change is correct

[wicked lodge]

e.g. you fix some obscure stairs bug and now you break slabs

or sth like that

correctness is basically "no seams and it looks reasonable"

nobody will notice if it's 1% darker 😄

[fallow ledge]

well yes, but as regression tests go, you'll know it's 1% darker and say "ok this is the new reference" 😛
but yes, if you're still in the phase where you're tinking with the algo this is not it

if you ultimately want "visual" confirmation

we can set something up where it renders multiple specific "setups" (scenes, whatever) into a collage

if you can even identify the edge cases that have problems

[wicked lodge]

e.g. this is bad

[fallow ledge]

Yeah that is something you could potentially "isolate" and automatically render into a large PNG along with other combinations that are "shit" like this

if that helps at all

just saying doing that is not that hard

[wicked lodge]

sounds like more effort than I am willing to do

[fallow ledge]

Probably

You could copy pasta the offscreen renderer with PNG capture, but yeah... how many scenarios like this do you even have built-out like this?

[wicked lodge]

I have 3 in mind right now

maybe I'll come up with more

they're kinda like game tests but with human verification

[fallow ledge]

Urgh that reminds me I still need to PR my system that even allows capturing game content in render targets to begin with. It needs a mixin right now.

[wicked lodge]

ok which of these looks more correct?

[fallow ledge]

uh

trick question?

is that ground block seriously fully lit on the left side?

or what am i looking at here

[wicked lodge]

you are looking at a slab below a sculk sensor

[wicked lodge]

it's not fully lit, but it's pretty lit yes

[wicked lodge]

no

I just need to know which one you find better looking

[atomic comet]

IMO the second one is more correct because the sculk sensor doesn't actually emit light

[wicked lodge]

good that's what I wanted to hear

[frozen loom]

Definitely the second one

[fallow ledge]

Erm yes, the first one really looks broken or like you placed a different block beneath it

[wicked lodge]

this confirms my choice of this nice center light formula

(i.e just use the center light and don't do any weird off-plane sampling)

[wicked lodge]

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

alright if someone wants to do a bit of testing that would be appreciated 😄

I will rebase the PR tomorrow

I'll probably have to remove COMPARE_WITH_VANILLA since it's quite useless with all the applied fixes

[atomic comet]

@wicked lodge Let me know on https://github.com/neoforged/NeoForge/pull/2105 what you want changed/cleaned up about the per-part AO implementation

[wicked lodge]

I'll have a look tonight

Probably checking out and playing around with your PR

[wicked lodge]

@atomic comet I find this code extremely taxing to read

somehow I can't make sense of it

ok now I got it; since it's in the AO function it is fine to default to true if per part AO is disabled

can't we just look up the config again instead of passing a boolean?

[atomic comet]

We could, same with the light emission. That's what I was referring to by "rather minor inefficiency" on the original PR

[wicked lodge]

same question for lightEmission, it seems that it's always passed as -1 if per part AO is enabled

we can cache the lightEmission inside the tesselateWithAO function, but I'm not a fan of passing it as a parameter

[atomic comet]

That's fair, yeah

That's part of why I explicitly mentioned that detail in the PR description

[wicked lodge]

well it got merged before I got the chance to have a look

otherwise I would likely have complained earlier 😛

[atomic comet]

Understandable 😄

[wicked lodge]

BTW I really don't like our calculateFaceWithoutAO hook

[atomic comet]

It's a bit ugly to say the least. But there was no better solution previously. I wouldn't be opposed to removing it in favor of the AO flag on the part

[wicked lodge]

wouldn't this make the json extension more complicated?

well I guess we'd get rid of the AO flag in the quad entirely; after all the unbaked model parameter should end up in the part

[atomic comet]

Right, I completely forgot that ExtraFaceData has an AO flag which applies to individual BlockElements and therefore has to be baked into the quad

[wicked lodge]

well, we can change the json format to remove that

that doesn't mean we should

imagine passing a singleton list to renderModelFaceFlat

[native mulch]

I think looking up the config is probably fast enough in practice

Remember anyone wanting max speed is using a reimplementation of this pipeline anyway

If profiling shows it's really a problem we can cache it

[atomic comet]

Yeah, that's definitely true

[wicked lodge]

BTW, do you want to make changes to the config also trigger a world remesh?

(it is already the case for the other AO-related config)

[atomic comet]

Sure, why not, would be reasonable to do so

Regarding the per-quad no-ao hook: could we potentially fold it into EnhancedAoRenderStorage#calculate()?

[wicked lodge]

we could also pull out the inside of the loop to another method

[wicked lodge]

we could but then it would be dependent on the enhanced pipeline being active

(and I will also want to target renderModelFaceFlat if I make any change to flat shading, since it should apply to all quads)

so I'm not sure it buys much in practice

[atomic comet]

Makes sense, yeah

[atomic comet]

I assume what you're going for is basically a renderQuadFlat() method and then changing the existing patch to something along the lines of this:

if (!quad.ao) {
  renderQuadFlat(quad);
  continue;
}

[wicked lodge]

yeah exactly

with 10 parameters unfortunately 😄

I am thinking of a place where I can apply the normal-based shading, so it would be good to centralize all the flat AO handling

[atomic comet]

The calling convention will not be happy, if you know what I mean

[wicked lodge]

is this some C-level stuff I'm too Java to understand? 😛

[atomic comet]

It's lower level than that. The platform's calling convention dictates how many registers can be used for function parameters and once you run out, further parameters spill onto the stack, which can cost you performance

[frozen loom]

Better than putting it on the heap at least

[atomic comet]

That's certainly true 😅

[fallow ledge]

i dont think java has the same limitations necessarily and inlining throws that out of the water too

this is some grandpa rant i am sorry: if you have a static function in C/C++ and call that the compiler can freely ignore calling conventions on that too, even if you dont use LTCG

[wicked lodge]

https://shipilev.net/jvm/anatomy-quarks/20-fpu-spills/

maybe they do crazy stuff like this too 😛

[atomic comet]

I'm fairly certain that this applies to JIT-compiled Java code as well in some way. I did some experimentation with un-lambda-ifying VoxelShape merging and in really hot code it was faster to allocate a "container" object with some of the context than passing down 12 parameters

[fallow ledge]

calling XMM registers "FPU" is ... isn't that just wrong

[bleak solstice]

Yes... The vector registers can be used for any kind of data. When you use SSE/AVX instructions that operate on them, that instruction's encoding specifies what kind of data is represented by the register.

[fallow ledge]

yes I know h

hence my comment 😄

I know originally XMX/SSE were mostly floating point oriented

but there are a bunch of non-float instructions for XMM registers by now

[bleak solstice]

The annoying part is that both INT32/FP32 instructions are executed on the thing called the "FPU" in some documentation

[fallow ledge]

given something as simple as memzero is going to likely employ SSE...

[bleak solstice]

Because generally speaking if you're implementing an FPU, you also need the silicon for integer bitwise ops and such, and well it's a disaster...

[fallow ledge]

FP32 is not really supposed to be used in AMD64 if you mean the x87 instructions

it's a bit confusing given the logic for that still exists

[bleak solstice]

x87 is not FP32, it's FP80

[fallow ledge]

Well not really though

yes, it's supposed to be

but the reality is different

[bleak solstice]

okay, sure, it's truncated and the whole x87 FPU is implemented in microcode

[fallow ledge]

We've had a lot of trouble for some physics simulation code porting from x86 to x86_64 due to the move from x87->SSE producing different results

but anyhow, we digress. I'd not call the XMM registers "FPU" at this point, really

[bleak solstice]

Java isn't actually required to use the platform's calling convention unless it's calling into native code. For that reason, some of the overhead of JNI is because it has to shift all the registers over so that it conforms to platform ABI.

So it's more than free to use more GPRs and VPRs if it wants... Not that it can generate efficient code.

https://wiki.openjdk.org/display/HotSpot/CallingSequences

[fallow ledge]

Yeah had to check, even the AES instructions use XMM registers

[bleak solstice]

It may be more efficient to pack these arguments into a class if they are getting passed around from method to method and not changing.

Much of Minecraft's block rendering code has method argument lists that are multiple lines long and it ends up being a problem for the functions that don't get inlined.

It would also be wise to cache that state object and reset it as necessary, since for very deep call trees, Hotspot's escape analysis will never allow the allocation to be elided.

Unfortunately the code generation won't quite be perfect since it will have to load the value every time you read it from the state object. You could of course grab the very important values from your state object and store them in a local variable, but then you're kinda back to square one and need to be mindful of register pressure.

[atomic comet]

Interesting. So I was sort of right but assumed the wrong root cause

[atomic comet]

@wicked lodge just pushed a commit to https://github.com/neoforged/NeoForge/pull/2105 removing the config and light value parameters as discussed earlier

[wicked lodge]

do you want to include a refresh on config change?

[atomic comet]

I knew I forgot something
Let me add that real quick

Done

[wicked lodge]

alright, who wants to go and review https://github.com/neoforged/NeoForge/pull/2078 ? 😄

[atomic comet]

I took a cursory look over it while you were working on it, just have the two monster classes left (i.e. EnhancedAoStorage and FullFaceCalculator). I might take a gander at understanding what the hell you're doing there tomorrow when I'm a bit more awake 😅

[wicked lodge]

I'm happy to explain (not today, I'm going to sleep). It would be good to have more eyes on this because we might want to adjust some tradeoffs 😄

[atomic comet]

I'll definitely be throwing questions at you tomorrow 😄

[lone pewter]

👀

[atomic comet]

I haven't dug into the code yet but, in an attempt to get rid of some of the overhead of AO for profiling (not that it made a difference), I experimented a bit with the new pipeline in my FramedBlocks dev env. The attached images are a comparison of vanilla AO (first image), old Neo AO (second image) and new Neo AO (third image) with a plane of slabs (all visible slabs are on the same Y level, just differing in the block half they are placed in). The top faces in the new Neo AO feel too dark to me.

[deft gorge]

given the fact that it's day in your world, it does feel too dark

[severe cipher]

I'm gonna be honest, both old and new neo AO look too dark to me

Though new is definitely way too dark

[frozen loom]

That's at least one reason why inset face blending sometimes produces a better result

[wicked lodge]

This is certainly caused by how I changed the inner light to be sampled

Vanilla's render is not too great either, it's missing the darker corners on the inside slabs

This specific case can be fixed by sampling outside if the 4 edges are solid

[frozen loom]

I wonder what the case looks like in Indigo, with/without enhanced AO enabled

[wicked lodge]

A key assumption that I am making is that faces inside of solid blocks are not visible

This is necessary to justify the corner hiding when both sides are opaque

So we can introduce discontinuities if nothing will ever see them 😉

[atomic comet]

I just noticed that you either need two layers of slabs or surround the bottom-half slab with full cubes instead of top-half slabs to reproduce this, otherwise you get a brighter result that's more like the old Neo AO. With only slabs you also need a 5x5 grid with the center slab being a bottom-half one, with full cubes a single bottom-half slab surrounded by four blocks is sufficient

[atomic comet]

Answering the last remaining TODO on the PR: yes, flat lighting needs adjustments. Even ignoring that the sloped granite surface is too bright, the dark rails look really stupid even though it matches what vanilla does for them. First image is the old Neo AO, second image is the new Neo AO

[atomic comet]

Irregular quads on blocks with a full-cube collision shape are incorrectly lit. Axis-aligned quads on the inside of such blocks are not affected (first image left vs center/right block).
Adding a solid block in certain positions "fixes" the lighting on certain vertices (second vs third image). With the "horizontal" version of that slope block this behaves even weirder (fourth vs fifth image)

[atomic comet]

In terms of the slope face being too bright: I think averaging the weighted and max brightness and weight with a bias towards the weighted value looks better. From some quick experimentation, 65/35 or even 75/25 for weighted/max looks good

[wicked lodge]

Interesting findings, thank you

Did you check that your clear blocks are correctly considered clear?

[atomic comet]

I'm assuming you are talking about insideClear in FullFaceCalculator#calculateFaceUncached(), in which case they are currently not because they are considered view-blocking. I can make them not view-blocking unless they are fully solid but that on its own won't fix it as sampleOutside is still true due to the full-block collision shape. Changing EnhancedAoRenderStorage#gatherAxisAligned() to use isSolidRender() (based on the occlusion shape instead of the collision shape) instead of isCollisionShapeFullBlock() for sampleOutside fixes it in combination with the view-blocking change on my end but I'm not entirely sure what the implications of that are.

[wicked lodge]

vanilla samples outside when faceCubic is true

and it is decided as follows:

isSolidRender would probably make more sense though

[covert plover]

For lighting calculations yes

Why would they use collision shapes here?!?

[wicked lodge]

no idea 😛

https://tenor.com/view/balthazar-marmotte-lache-pas-gif-6477015835627170837

the feeling of fixing AO bugs

[covert plover]

😄

[atomic comet]

[covert plover]

I looked at this twice in my career already

And decided both times

Nope

Not for me

Somebody else can fight this battle

Hat of to you that you my good sir

[atomic comet]

The computed value in question would be f8 (line 901) in ModelBlockRenderer.AmbientOcclusionRenderStorage#calculate(), right?

[atomic comet]

Vanilla does not consider the sloped face cubic

Doing this to match the way vanilla decides whether to sample outside fixes it as well and doesn't require switching to isSolidRender() (actuallyAxisAligned is passed true in calculateAxisAligned() and false in calculateIrregular). Once again not sure of the implications

[covert plover]

The collision shape check there seems majorly off

I know vanilla uses it

But it still feels wrong

[atomic comet]

Once you're certain that the face is both axis-aligned and on the perimeter of the block volume instead of somewhere in the middle, that shape check isn't too far fetched

[covert plover]

No, but sides which are partially see through can be on the perimeter

And would result in wrong calculations no?

[atomic comet]

No, all axis-aligned faces, regardless of perimeter or not, render fine in both vanilla and the new AO implementation

[covert plover]

Hmm okey

Mis understood the issue then

Well if you mean this is fine

Then yeah

Seems good

Still feels wrong though

[wicked lodge]

oh right, I changed the check to be per-vertex

the sloped face is not cubic, but since it is an irregular face we have to consider its projections onto axis-aligned faces

[wicked lodge]

they both have the same logic at the end of the day, for axis-aligned faces

the only difference being how the new AO does not depend on vertex winding (but this is questionable tbh, so might be reverted?), and how the full face light is computed

I wonder what the light level is inside your slope+glass block

[atomic comet]

What's interesting is that this change to outside sampling also resolves the brightness issue on sloped faces I pointed out here ^

[atomic comet]

14 sky, 0 block

[wicked lodge]

hmm, how can it then render so dark 🤔

[wicked lodge]

wait are we talking about flat shading now?

[atomic comet]

No. There's two brightness issues in that image. The rails are too dark which is due to flat shading being unchanged and the granite slope behind it is too bright. The latter is what I'm referring to now. I should have clarified that

[wicked lodge]

that looks fine, no?

[atomic comet]

Btw, I just noticed this, is it intentional that the corner indices 0 and 1 are flipped here (bottom of FullFaceCalculator#calculateFaceUncached()):

[atomic comet]

The brightness of the slope face is a bit too close to the brightness of the top face IMO

[wicked lodge]

ah sure

[wicked lodge]

that's how vanilla does it

[atomic comet]

This is what it looks like on my end

[atomic comet]

Hmm, odd

[wicked lodge]

yeah it's super weird

there's so many different uses of the 0-3 indices

[atomic comet]

I gotta leave for now. I might get to do some code review later tonight, otherwise I'll look into it properly tomorrow

[wicked lodge]

the neighbors, the corners (to sample corner blocks), the actual AO corners, the quad vertices

[wicked lodge]

essentially this means that irregular faces never sample outside

which does match vanilla!

I am getting this out of the box

this is using the FB build that you gave me ~ a week ago

are you sure you are using the latest version of the PR? 🤔

my last commit is

commit e82a231938b4a04d490227a1fa181e75f64822a5 (HEAD -> enhanced-ao, origin/enhanced-ao)
Author: Technici4n <[email protected]>
Date:   Fri Apr 4 00:46:45 2025 +0200

    Fix formatting

[wicked lodge]

this is vanilla (in NeoForge env, so with the oversampling bug fixed); with a grid of full blocks

and this is vanilla with the slabs; again with the oversampling bug fixed

[atomic comet]

I pulled the PR yesterday afternoon, so I definitely have that commit

[atomic comet]

Make sure you have two layers of the slab grid, having more slabs and therefore non-empty, non-full blocks below seems to be what makes the difference

[wicked lodge]

this in the air is enough

(neo first then vanilla)

[atomic comet]

You have double slabs in there which are effectively equivalent to full blocks, that's why it breaks without the additional layer (I mentioned that last night). I can't see how the slab setup on the ground is done

[wicked lodge]

when you compared against vanilla, you were still using NeoForge right?

[wicked lodge]

the discrepancy here is caused by vanilla not checking corners correctly(?), which we do fix (// Neo: remove move() to avoid oversampling (MC-43968))

[atomic comet]

Yes, I tested in the FramedBlocks dev env, the first images I sent of the stone slab grid aren't actually vanilla stone slabs 😄 I did make sure that the same happens with vanilla slabs though

[wicked lodge]

this is NeoForge; what am I missing?

a second layer maybe

[atomic comet]

Yes, you're missing the second layer

[wicked lodge]

exactly the same?

(i.e copy paste at z=~-1?)

[atomic comet]

Yup

[wicked lodge]

okay I see

[wicked lodge]

ok it's caused by the new mixing formula

the upper half slabs have a light level of 0, but they are clear

vanilla chooses to ignore their light level because it is 0

[frozen loom]

Edge clear boolean computation can be simplified like in Indigo

The correct pos and state to check are already available earlier in the code when they are used to get light and AO

[wicked lodge]

the slab case will be considered WAI; it's a vanilla bug that we are fixing more than anything

because of vanilla's blending formula, when the light level drops to 0, suddenly the middle block gets brighter...

[wicked lodge]

you mean compared to vanilla?

indeed they can be

I wonder why mojang did it that way

[frozen loom]

Because Mojang offsets them further

[wicked lodge]

yeah...

[wicked lodge]

I just added back normal-based shading for flat quads

[wicked lodge]

and also did this

based on a report by Lolo I had a look at what happens when gamma is reduced to the minimum (brightness: moody)

left is the new pipeline, right is vanilla

it's crazy how many things change

[atomic comet]

I had to binary search the commits I made since I gave you that build but I found the discrepancy: when I initially started testing the PR in my dev env, the lighting on sloped faces seemed odd to me. Since I recompute the baked normals and "light face" and zeroing out the baked normals there made a difference, I compared my normal recompute to yours and then adjusted mine to match yours. My previous implementation (which the build you have still uses) truncated the components after multiplying them with 127 while yours and my new one round them after the multiplication: https://github.com/XFactHD/FramedBlocks/commit/d1009d4418b6eac7fe8763c99af266c3920c7602.

[crisp dew]

are you all doing the fractional light coords properly? I hope you remember it's 4.4 fixed point (0.0 to 15.0 in increments of 1/16th) that stumped us for far too long back in 1.13/1.14

[atomic comet]

Hmm, good point. Some of the light blending currently uses the LightTexture helpers which cut off the decimal part while vanilla's light blending in ModelBlockRenderer.AmbientOcclusionRenderStorage.blend() does not.

[crisp dew]

yep that causes things to be a little bit darker

[atomic comet]

A little bit darker is an understatement. Changing all light interpolations to take the fractional part into account fixes the slope lighting I mentioned here: #1355572979320230119 message. Combining it with the outside sampling fix I mentioned yesterday (#1355572979320230119 message) avoids making the sloped faces (regardless of full-block or not) too bright

[frozen loom]

Indigo always respects the fractional part so I don't know why Tech undid those changes

[wicked lodge]

I use the LightTexture helpers because why not

(I mean that would be a reason)

[wicked lodge]

So what's the issue? Because the build you gave me seems to give sensible results, and I doubt that just rounding the components differently would make a difference?

[atomic comet]

The rounding difference did somehow make the difference, nothing else changed between the builds with and without that linked commit

[wicked lodge]

But the difference is between "completely broken" and "looks reasonable"

[atomic comet]

Yes. I have zero clue how the rounding difference exacerbates the absence of the fractional part of the light values to such a degree

[atomic comet]

I found why and in hindsight it's quite obvious: on a 45 degree slope, the rounded normal components end up as 90/127 while the truncated ones end up as 89/127. The resulting axis weights are ~0.502 and ~0.491 respectively. The former adds up to >1, causing the final weighted brightness to overflow and become zero once masked by the decomposition in the lerpLightmap() after the axis loop. The only reason why it's not pitch black is the lerping between the weighted and the max brightness.
The final weighted lightmap values before the lerping with the max lightmap value are as follows:

• Truncated normals: 0x00E0_0000
• Rounded normals: 0x0100_0000
• Truncated normals with full-precision light: 0x00EC_0000
• Rounded normals with full-precision light: 0x00F2_0000

[wicked lodge]

Are you sure about this last one?

[wicked lodge]

Nice find! A bit sneaky I would say 😄

[atomic comet]

Let me double-check 😄
It certainly does look strange

Yeah, it was bogus, fixed it. I probably forgot to hotswap one class

[wicked lodge]

ok yeah looks better now

so what's the matter with fractional light coordinates?

[atomic comet]

They somehow save us from the overflow (for the most part at least), that's all I know so far

[wicked lodge]

which part of sideLightmapA + sideLightmapB + cornerLightmap + insideLightmap >> 2 & 0xFF00FF looks aware of fractional light coordinates?

(this is derived from vanilla code)

ah but that's already aware of fractional coordinates

[atomic comet]

That line is implicitly aware for fractional lightmap values by virtue of operating on the packed value directly

[wicked lodge]

FF is 256

[atomic comet]

Mhm

[wicked lodge]

wtf is this

[atomic comet]

The parts that are currently not aware of fractional light values are EnhancedAoRenderStorage.lerpLightmap(), EnhancedAoRenderStorage.maxLightmap() and FullFaceCalculator.blend()

[wicked lodge]

ah, LightTexture#sky is not aware of them

but LightTexture#block is??

that's a forge patch

[atomic comet]

Nope, neither are. block() explicitly cuts off the fractional part with that 4bit shift

[atomic comet]

I dug into that earlier, that patch is unnecessary since 1.21.2 but nobody noticed. Before 1.21.2, block() did value >>> 4 & 0xFFFF which caused the fractional part of the skylight value to be included in the returned value.

[wicked lodge]

15 is 0x000F

[atomic comet]

Yes, and before 1.21.2 it was using 65535

https://github.com/neoforged/NeoForge/blob/916bfcbc540abf1a1b1ba4162905a28aa754894d/patches/net/minecraft/client/renderer/LightTexture.java.patch

[wicked lodge]

wouldn't this return only the fractional value?

[atomic comet]

No, it would only return the integer part. The lightmap values are layd out as int.frac in 4.4 fixed point

[wicked lodge]

4.4 meaning 4 bits + 4 bits?

[atomic comet]

Yes, 4 bits integer + 4 bits fraction

[wicked lodge]

so that would be 0xij where i is the integer part and j is the fractional part?

[atomic comet]

Mhm, or 0x00ij00ij in the full packed layout

[wicked lodge]

in other words 0xF0 would be integer 15 and 0x0F would be fractional 15

[atomic comet]

Yup

[wicked lodge]

ah right the shift by 4 only shifts by 1 in hex

ugh

so indeed the LightTexture methods are not suitable

amazing

should we maybe patch new ones in that return the full range?

[atomic comet]

Yup. Vanilla also ignores those helpers for packing/unpacking in ModelBlockRenderer.AmbientOcclusionRenderStorage.blend() in order to support fractions

[atomic comet]

That's a thought I also had

Something along the lines of blockWithFraction(), skyWithFraction() and packWithFraction()

[wicked lodge]

isn't it 0xijij?

no it's not

I confused myself again with these bit shifts

[wicked lodge]

gonna remove this patch in the process then

do you have an updated FB build by any chance?

[atomic comet]

Yes, one sec

[wicked lodge]

vanilla looks awful btw 😄

ok reproduced the darkness issue; do you know why the normal slopes are fine?

now with fractional coordinates everything is fine (even without considering that the lerp might overflow; because it doesn't anymore)

[crisp dew]

nice

[atomic comet]

I'm not sure to be honest. I'll have to look at the computed light. My best guess is that inside vs outside sampling somehow makes the light value per axis smaller by just enough to not overflow

[wicked lodge]

ok pushed the fractional coordinates

I think it's more correct to round in the normal computations, so let's do that

(it means that we are at most 1/2 * 1/127 away from the real value instead of 1/127)

ah but it already rounds

ok yes it was just old FB who didn't; got it

[atomic comet]

Yup, except that single slopes are slightly too bright (double ones are too but partially for another reason on my end related to shade brightness). The fix for that is to not sample outside if the quad is not axis-aligned which would match vanilla

[wicked lodge]

I don't have an opinion on that tbh

how do you see that they are too bright?

[atomic comet]

Place a slope and then a full cube right next to it

[wicked lodge]

e.g. this?

[atomic comet]

It's visible without a block above. First is with the fix, second is without. With the fix the slope gets shaded a bit by the neighboring cube (which looks better IMO), without the fix the lighting is pretty much flat and very close to the upward face in terms of brightness

[wicked lodge]

yeah, first looks better indeed

ok let's do it; I'll change the code a bit to simplify it in the process

[atomic comet]

👌

Speaking of flat, I was gonna mention that in the PR review as well but you have a sneaky bug in your flat shading calculation: you forgot to cast the normal components to byte in order to sign-extend them. This causes them to always be considered positive. This is only noticeable on the underside of blocks because they are really bright

[wicked lodge]

I am getting seams

maybe I did it wrong though; it would be good to double-check

I suspected something like this would happen because this causes a discontinuity (the 2 lower vertices of the upper sloped block are sampling in a different plane than the 2 upper vertices of the lower sloped block)

[atomic comet]

Hmm, yeah, that's annoying

[wicked lodge]

maybe we can change the irregular mixing formula to include less of the max

[atomic comet]

Yeah, that's probably the easiest option. 75% weighted / 25% max looks pretty good

[native mulch]

Is the slope applying darkening to the adjacent sides of the stone blocks or is that just how directional shading normally looks?

[atomic comet]

Mostly directional shading and a tiny bit of shading from the slope

[wicked lodge]

max weight of 0.5 on the left; max weight of 0.75 on the right

0.75 it is

[atomic comet]

👌

[wicked lodge]

(I meant 0.25)

[atomic comet]

I figured 😄

[wicked lodge]

pushed

[wicked lodge]

hmm

okay that should be fixed now

I am going to sleep now, let me know if anything else comes up; huge thanks! 😄

[atomic comet]

Will do 👍

[atomic comet]

I wish Arrays.setAll() would return the provided array, then you could just do private final AoCalculatedFace[] aoFaces = Arrays.setAll(new AoCalculatedFace[24], $ -> new AoCalculatedFace());

[native mulch]

how about Util.make?

[atomic comet]

If you mean wrapping Arrays.setAll() in Util.make(), then sure, that would be an option

[wicked lodge]

Valhalla will save us all one day

[deft gorge]

some days everytime we mention Valhalla's arrival feels like

[candid moss]

random java question: is the dollar sign there just for throwing away the identifier? 🤔

because if so, TIL

(used to C# where you discard with an underscore)

[crisp dew]

java has no discard keyword

[deft gorge]

Java used to allow identifiers consisting of a sole underscore

but they banned it, probably so in the future, they can implement a feature that uses the sole underscore for some special meaning

(such as, for example, "discard this value")

[crisp dew]

they reserved _ for maybe a future discard but I don't think they have done anything about it yet

[candid moss]

as did C#, and they banned it for the same reason 🤣

though by "banned" it's more like a soft "banned"

anyways, thanks for the info

I didn't realise $ was a valid identifier in this context

[crisp dew]

it mostly is. we don't need valhalla to make fast code but we need valhalla to make pretty code that isn't as slow

[deft gorge]

most of the time, I just use a random lowercase letter that isn't already used in scope

or name it somewhat significantly but not too descriptively

like, I'd potentially name the variable a or arr in the example above by XFact

but I digress

[deft gorge]

actually, https://openjdk.org/jeps/456

JEP 456: Unnamed Variables & Patterns

Status Closed / Delivered
Release 22

Enhance the Java programming language with unnamed variables and unnamed patterns, which can be used when variable declarations or nested patterns are required but never used. Both are denoted by the underscore character, _.

we're one Java version away from greatness /j

and yes, this does encompass lambdas,

Here are the examples given above, rewritten to use unnamed variables.
[...]

• A lambda whose parameter is irrelevant:

 ...stream.collect(Collectors.toMap(String::toUpperCase,
                                    _ -> "NODATA"))    // Unnamed variable

[crisp dew]

oh fun

they DID do something about it!

[deft gorge]

but we can't use it until and unless Mojang graces us with the next Java LTS

which will still be later this year, in September

maybe

[atomic comet]

I hope flexible constructor bodies with the ability to init fields before super are done by Java 25

[deft gorge]

they've not yet submitted a JEP for flexible constructor bodies, so depends if they do and its unpreviewed

for reference, the previous JEP in Java 24 is the third preview

[atomic comet]

[deft gorge]

oh wait no

they have submitted a JEP for it -- and its non-preview

https://openjdk.org/jeps/8344702

so, uh good news!

[crisp dew]

nice

hopefully makes it into the java25 cycle

[deft gorge]

very likely so

I'd say, at least

[wicked lodge]

Dollar sign is a valid character

It's used for inner classes

The decompiler uses it for variables whose type is an inner class

[wicked lodge]

@atomic comet review comments addressed

I also updated the PR description with an exhaustive list of the changes: https://github.com/neoforged/NeoForge/pull/2078

[atomic comet]

Looks good to me. A couple points:

• Make sure to revert the MixinExtras dependency
• Did you test the ME Drive model?
• This remains an "issue" with the new AO: https://github.com/neoforged/NeoForge/issues/1093. Do we care?

[wicked lodge]

1. good point
2. no. I will do it in a bit
3. weird; it should resolve to something like 0x0c with fractional coordinates whereas in vanilla the whole quad gets 0x10 because of the mixing formula removing 0 values

3 is yet another example of why simply checking for 0 creates inconsistencies 😄

[atomic comet]

With a single level 1 light block the "inside" lightmap of the solid block right below it is 0x10 which gets averaged down to 0x04 due to the two relevant edges and the relevant corner all being 0. I need to place two neighboring level 1 light blocks around a given corner to get a visible light level at the corner (doesn't matter whether it's at the two adjacent edges or at one of the edges and the corner).

[wicked lodge]

huh yeah 0x04; not sure why I wrote 0x0c I was thinking c is the 3rd letter probably

[atomic comet]

Hehe

[wicked lodge]

looks correct to me

[atomic comet]

👌

[atomic comet]

0x0c is, coincidentally, a relevant value: it's what you get as the result with two neighboring blocks which is the point where it becomes visible (it's probably anything > 0x07)

[wicked lodge]

ah yes ok

well 🤷, working as intended then

I just removed mixin extras from runtimeOnly again

[atomic comet]

In that case, feel free to add the issue I linked as an additional sub-issue to the one your PR will close (in the hopes that it will automatically close all sub-issues)

[fallow ledge]

check it with adjacent full blocks

the problem was that it ignored our desperate attempts at setting the cullface/light sampling direction of the internal faces to the front

instead it continued to sample the block light for the internal panel on the right from the block to the left (which, if occupied by a solid block, hat light level 0)

[wicked lodge]

looks reasonable to me

[atomic comet]

Definitely looks miles better than the old pipeline. I believe the vanilla AO wouldn't shade it at all though

[wicked lodge]

vanilla makes the inside too dark in the left case

not sure why

[atomic comet]

[atomic comet]

Ok, now I'm thoroughly confused

This is what it looks like in 1.21.1 with vanilla and the old Neo pipeline respectively

[wicked lodge]

god the vanilla AO on the grass behind looks quite bad

[atomic comet]

It does

[wicked lodge]

isCollisionShapeFullBlock is true for the drive, so any inside face is cubic

[atomic comet]

That makes sense, yeah

[wicked lodge]

the center light is therefore 0

so if you enclose the adjacent block it gets worse 😄

when the adjacent block is fully enclosed it also looks bad with the new enhanced AO

the solution is to make sure that isCollisionShapeFullBlock is not true

I would say WAI though

[atomic comet]

i.e. inset the front face of the shape by one pixel

[atomic comet]

Sounds good to me

I'm gonna put my approval on the PR tomorrow, I'm on my way to bed now

[wicked lodge]

massive thanks 🙂 good night

[fallow ledge]

hm, we can disable AO for those faces if needed, actually

it was also broken for non-ao IIRC

[wicked lodge]

correct

that is expected

[fallow ledge]

what is expected?

[wicked lodge]

we need to make sure that isCollisionShapeFullBlock is false

[fallow ledge]

that the old neo pipeline was borked? 😄

[wicked lodge]

otherwise the light will be "forced" onto the block face

[fallow ledge]

because it was really borked

for AO or non-AO?

[wicked lodge]

both

[fallow ledge]

it worked in Vanilla though

does it completely consider the cullface in your logic?

[wicked lodge]

the cull face is not used for AO at all

[fallow ledge]

and non-AO?

[wicked lodge]

not used either

[fallow ledge]

Are you sure about that? Since IIRC that was what made it actually work for vanilla

[wicked lodge]

once this goes through I should really write a practical guide on "common" AO issues and how to fix them

[wicked lodge]

well it doesn't work in "vanilla" 1.21.5

[fallow ledge]

definitely did in 1.21.1 though 😄

And yup doesn't in 1.21.5

i didn't actually think vanilla had changed anything?

[wicked lodge]

I have to sleep but you are making me curious

[fallow ledge]

Okay no, it still works fine with AO disabled in 1.21.5

[wicked lodge]

let's have a quick look

[fallow ledge]

It's possible this is due to our old rendering of the drive always force-disabling AO

[wicked lodge]

huh

[deft gorge]

go shartte! delay Tech's sleep more by nerdsniping him! /j

[fallow ledge]

1.21.5 with AO disabled at the model level looks suspiciously like the 1.21.1 screenshot (that had no mods)

it's possible something (or we inadvertently) were disabling it, but I am not sure

[wicked lodge]

BTW you are right that the cull face makes a difference

[fallow ledge]

It was the detail that made it work and that was ignored by the experimental pipeline in NF (which made it break)

[wicked lodge]

for flat lighting specifically

[fallow ledge]

Yes, because in that case, there is at least some confidence that if a face is removed if side X is occluded, then the lighting from side X is relevant for that face

[wicked lodge]

I'm trying to understand why it makes a difference 😄

if a cull face is set it will sample the light outside of the block, always

[fallow ledge]

hm you sure? i thought it was also sampling outside the block if you didn't set the cullface

just that it sampled from the direction the face was facing

[wicked lodge]

if you don't set a cullface, it samples using the closest direction to the face

[fallow ledge]

yes exactly, which is in the solid block for this inside face

[wicked lodge]

it will sample outside the block if the face is axis-aligned and:

• it is on the edge of the block
  OR
• the block has a full collision shape

otherwise it will sample inside

could it be that the face is not perfectly axis-aligned? 🤨

[fallow ledge]

it's a JSON block model

how would that be possible

[wicked lodge]

        "north": {
          "uv": [15, 1, 16, 15],
          "texture": "#6",
          "cullface": "north"
        },
        "east": {
          "uv": [0, 1, 16, 15],
          "rotation": 180,
          "texture": "#inside",
          "cullface": "north"
        },

that's how

it does have a cull-face set, on a completely different direction

so it will sample the light in front of the drive

[fallow ledge]

Yes...? That's the point of setting that

[wicked lodge]

crazy but kinda sensible 😄

[fallow ledge]

how does that relate to it not being axis aligned

And you are just repeating what I've been telling you the entire time 😄

[wicked lodge]

it doesn't matter what the shape of the face is for flat AO if a cullface is set

[fallow ledge]

I set the cull faces to do that, and exactly that

And the NF experimental pipeline did not implement this -> bug 😄

hence my suggestion of checking this model hehehe

[wicked lodge]

yeah true, this one is particularly sneaky 😄

well let me test again

[wicked lodge]

the experimental pipeline was so complicated for flat lighting; I really don't get it

there is only one patch that is needed for flat lighting: using normal information for irregular faces

[fallow ledge]

That what I'd also have thought, since the code path to how vanilla samples the cull-face direction for light is actually quite clear 😄

[wicked lodge]

the experimental pipeline used inheritance soup to achieve its goals

there was a base QuadLighter class that did a bunch (!) of stuff

and then FlatQuadLighter was an extension of that

probably ignoring most of what was inside of QuadLighter?

[fallow ledge]

Anyhow note for us (AE2): disable AO for the inside faces

now that we can do it per-face (that is a NF extension?)

[wicked lodge]

yes it is

[fallow ledge]

It's possible previously our FRAPI-at-home forced that flag to false 😄

[wicked lodge]

or make sure that isCollisionShapeFullBlock is false 😛

[native mulch]

Agreed, especially while you remember how the pipeline works 😛

[wicked lodge]

Don't forget 😄

[atomic comet]

I won't 😄

[atomic comet]

@wicked lodge one final thought that just came to my mind: should we maybe rename the config option to ensure it actually gets enabled for everyone when they update?

[wicked lodge]

Ah that's a good point

[wicked lodge]

Well tbh I don't want to update all the translations

Does it even matter? Most people haven't used 1.21.5 yet

[atomic comet]

You could get away with just changing the config key. The translations are separate from that and, crucially, do not include any reference to being experimental (which is something I really want to get rid of)

[wicked lodge]

Ok let's do that

[wicked lodge]

Alright done

[hoary thicket]

just use SSAO

[bleak solstice]

hi, just checking in

is there anything that other rendering mods (i.e. Sodium) need to do differently with these changes?

I think our light pipeline currently solves most graphical issues (being an improvement upon Fabric's), but the appearance is slightly different from NeoForge with these changes.

I have been sick the past week and haven't been able to follow super closely

[lone pewter]

It's based on the new Indigo formula

[wicked lodge]

TLDR the enhanced AO now implemented in NeoForge looks better than Indigo/Sodium IMO

The most important change is not blending between inside and outside light for inset faces

To answer your question: Nothing to change on your end in principle

I have considered adding some BlockState extensions to give blocks more control over the lighting but it hasn't been needed so far

[blazing bloom]

is the stair back issue fixed?

[atomic comet]

Yes

[lone pewter]

I wonder if this pipeline fixes all light bugs

[bleak solstice]

fixing "all" light bugs is a tall order

[bleak solstice]

okay, thanks, I will take a look. I know Pepper talked to me about changes going into Indigo and I said we'd look at implementing them for next release.

[native mulch]

@wicked lodge this is probably WAI, but I figured it's worth noting - the new pipeline seems to strengthen path shadows a bit in some configurations, which makes asymmetry issues in vanilla more obvious (left is Neo, right is vanilla)

[wicked lodge]

Vanilla vanilla or vanilla with the patch by NeoForge?

[native mulch]

Completely vanilla instance

[wicked lodge]

One explanation would then be vanilla using the wrong offset to decide whether corners should be checked or not

I.e. here it decides to check the corner because of the path block 2 blocks away

Try replacing that block by grass maybe

(2 blocks "above" your cursor)

[hoary thicket]

paths like that have AO in the first place? wtf...

[native mulch]

Ok yeah the behavior seems to be identical with the Neo version before the PR was merged, i.e. with the offset fix but without the rewritten pipeline, so this is not a regression

[native mulch]

This has no effect on the original path block with either pipeline

[blazing bloom]

always have, it's physically a pixel below solid blocks

[hoary thicket]

i mean, i understand why its happening, but also that seems like case where maybe it shouldn't (art/look perspective, not technical)

[wicked lodge]

Indigo and Sodium remove that shadow completely but then people complained about it 😄

(+ removing it introduces other issues)

[severe cipher]

This... does seem like an issue. Since that left one looks... bad

[wicked lodge]

It's unfixable I'm quite sure you can trigger it in vanilla too 😛

You just need a solid block 2 blocks away

Hmmm I think that we have a problem if a sloped quad is split in two

I haven't checked though

[wicked lodge]

i.e. a sloped block like this, but with the sloped quad split in two (i.e. one part above y=0.5 and one part below)

[atomic comet]

Could be worse

[crisp dew]

I was noticing how every screenshot shows a vertical test and wondered if anyone has tested horizontal wedges, but now I see that mess in the back full of wedgy blocks

[atomic comet]

[vapid bough]

that's just all blocks, right?

[atomic comet]

Mhm

[vapid bough]

as can be seen on the cf page too

[wicked lodge]

Hmm

This one has a split face?

The thing is that vertices at the edge will get the outer light and all others will get the inner light

I suppose that you don't notice that much since it remains continuous

It should get more noticeable if one quad is very short and another one very tall

[atomic comet]

Yes, it's using a connected textures model

[wicked lodge]

Yeah ok that's reasonable

[lone pewter]

I suppose it's normal, but the light isn't completely Smooth when a magma block is close to a FramedBlocks “Framed Inner Corner Slope”.

[fallow ledge]

That still subjectively looks good imho

[lone pewter]

yeah

[wicked lodge]

The discontinuity might be caused by the change of angle

Same as if you go from one side of a block to another, sort of

[obsidian gust]

yeah vanilla having lighting be stronger on certain sides of blocks is annoying lol

Build a wall "why is this wall so dark???"

change the wall's direction and now it is lit up

[lone pewter]

The light coming from the bottom of the lecterns is broken, it's fixed in Indigo, but it's also broken in Vanilla.

(but it's slightly more broken with the pipeline than with Vanilla).

[blazing bloom]

yeah but, even with AO i suspect that blocks with homogenous textures would still look weird without it

i think i once saw someone complain specifically that their model with a 45 degree rotated cube looked weird because two of the faces were considered to be in the same direction for that purpose

[since while entities get it smoothly varying by actual face angle, for blocks it's 'whatever cardinal direction the face is closest to']

so their southeast and southwest faces both got the "south" normal

[wicked lodge]

Might have forgotten to clamp