19w42a rendering and layers etc

[liach]

Signed-off-by: liach liach@users.noreply.github.com

[ChloeDawn]

BreakingBlock/CrumblingBlock?

[liach]

This isn't a block but info on block breaking, so i call it a breaking progress

[Runemoro]

I'd call this BreakingBlockInfo, since it's more than just a progress, it also has the position of the block.

[grondag]

Only about 1/3 through this. I don't understand many of the remaining classes well enough yet to comment. Will keep going.

[grondag]

This appears to be a normal matrix based on usage. Suggest calling it peekNormal.

Alternatively, call it normalMatrix and make peek be modelMatrix. You lose the stack semantics that way, but pop returns void so the operation is always a peek no matter what.

[grondag]

normalMatrix is closer to actual usage. It's used to transform normal vectors during buffering.

Suggest transformation would be clearer as modelMatrix.

That's still somewhat confusing because these almost certainly aren't the model/normal matrix transforms used in rendering. All of this is pre-buffer. But really don't know what else to call them.

[grondag]

Should be called something like fixedColor instead of fallback.

Fallback would make sense for a color that is used when no color is provided, but vertex consumers will throw IllegalStateException if an attempt is made to set the color when this flag is true.

Similarly, suggest the fallbackColor method should be fixedColor or similar, and the variables should renamed with a fixed prefix.

[liach]

I once called it fixed, but it isn't actually fixed after another look

In detail, this fallback color is set for entity rendering (outline in glowing). Though in one subclass the color can no longer be set after this fallback is set, in another subclass this is only the fallback (i.e. the color gets initialized to this fallback color after the next element call)

[Runemoro]

Does this boolean mean that the fallback color is set? If yes, then I think fallbackColorSet or hasFallbackColor would be clearer.

If I understand correctly, this is the color that a vertex will have if no color is specified? In that case, I'd name it defaultColor.

[liach]

In one vertex consumer, when this color is set, setting color again is disallowed; in another, this is just the default color.

[Runemoro]

setting color again is disallowed

You mean setting the vertex color is disallowed, or setting the fallback color is disallowed? In the second case, I don't see what's wrong with defaultColor.

[liach]

setting vertex color is disallowed in one case.

[Runemoro]

I just looked at the code, and there's three subclasses of AbstractVertexConsumer:

• BufferBuilder, which throws an IllegalStateException when trying to set the color of a vertex
• MatrixVertexConsumer, which throws an IllegalStateException when trying to set the color of a vertex
• FixedColorLayeredDrawer.VertexConsumer, which completely ignores calls to color (they do nothing)

So it's not a fallback, but rather a fixed color, like @grondag suggested. By setting it, all future vertices will automatically be colored, and you won't be able to manually color them.

[liach]

Changed.

[grondag]

drawStart acts as a drawLimit most of the time. Alternatively, it could be nextDrawStart Its only used as the start of the current draw during pollArrayDrawParameter and then is immediately updated to point to the start of the next draw.

[liach]

nextDrawStart should be better.

[Runemoro]

packUv, if this is just packing two numbers into a single int (u << 16 + v)?

[Runemoro]

What does this do differently from saveScreenshot? Maybe a more precise name would be better?

[liach]

just like the bindTextureInner change in #870

[Runemoro]

What does this do exactly? If it just gets a column from a Matrix4f, why is it in the Vector4f class, and why does it take so many parameters?

[liach]

It is a private method used for applying the matrix transformation (multiplying matrices).

[Runemoro]

So it doesn't just get a column? It should have a better name in that case.

[liach]

this is more like calculate a new column. int is target row in matrix, the matrix is the matrix, and 4 floats are the original columns in the vector 4f (assuming the vector 4f is a 1 row by 4 column matrix)

[Runemoro]

Then I think it should be something like getMatrixProductColumn. getColumn is too vague.

I'm still confused though, the result of multiplying a matrix by a vector is a vector, so why does it have several columns?

[Runemoro]

What...? This is a builder class. ToTagger would be a method name, and what's a tagger?

[liach]

This is not a builder. This is something that builds a NB tag, named like guava's ToStringer

[Runemoro]

Interesting. I'd prefer something like PatternTagBuilder or PatternListSerializer, but I guess ToTagger works, if guava's using it too.

[Runemoro]

This doesn't return just the draw parameters, but also a copy of the ByteBuffer containing the vertex data (so all the data that's needed to upload the buffer).

I think takeData (or getDataAndClear) would be much better.

[Runemoro]

VarArg?

[liach]

This is for transpose usage, but this arg is not properly passed to the constructor it delegates to (true instead of this param is used, but luckily this param is always called with true so no error emerges)

[Runemoro]

I understand, but how does VarArg mean that the parameter is not being passed? To me VarArg would mean it's a var arg (boolean... transpose).

no error emerges

Someone creating a new Matrix3f(..., false) would be surprised when they find out that their matrix is being transposed even though they passed false to the constructor. I think the name should make it clear that it's broken, so that someone doesn't accidentally use it. Something like IgnoredAlwaysTrue would work.

[liach]

I've resolved all the requests above and am asking for another roubd of review.

[modmuss50]

Looks fine to me, im no expert on this so I may well have missed something.

[sfPlayer1]

Looks like an improvement from a cursory look, can always tweak it later. I hope the xyzw/rgba/abcd associations are ok...