@grondag I'm not getting rid of the BlockEntity split without a good argument. Look at MixinClass853 - iterating over all BlockEntities in a chunk is probably faster than having every Block with a BlockEntity get it from the world view... For Class853, it's an array lookup + hash lookup per getBlockEntity. Not sure which approach is more likely to be faster.

@grondag I'm not getting rid of the BlockEntity split. Look at MixinClass853 - iterating over all BlockEntities in a chunk is probably faster than having every Block with a BlockEntity get it from the world view...

Hmm, I did see that. That's actually what prompted me to suggest removing it. You already iterate through all the blocks in the MixinClass853 constructor before checking the Block Entities.

Given that you've already queried every block in the chunk to ask if it has render data, and those blocks should already know if they rely on a BlockEntity for render data, why not delegate the BlockEntity lookup to the Block, vs doing it in a separate loop?

I suppose if you want to query the BlockEntities first, and then avoid checking blocks if you already have the render data, but then you'd have to add a check at each at position in block loop, and you'd still be doing two iterations.

I suppose if you want to query the BlockEntities first, and then avoid checking blocks if you already have the render data, but then you'd have to add a check at each at position in block loop, and you'd still be doing two iterations.

The difference: You can iterate BlockEntities by the Map's values(); by iterating Blocks and having them get their BlockEntities, you have to do an additional array+hash lookup every time.

I'd like to benchmark both approaches at some point, but I'm not sure how to best do it.

The difference: You can iterate BlockEntities by the Map's values(); by iterating Blocks and having them get their BlockEntities, you have to do a hash lookup every time.

This is true. Might be premature optimization but I won't tell Knuth if you don't.

If you think most mod authors are likely to provide render data from BlockEntity, might be worth doing the Block Entity loop first, and then skip the blocks calls when you have non-null render data in the array. Hard to say which way will be more prevalent.

If you think most mod authors are likely to provide render data from BlockEntity, might be worth doing the Block Entity loop first, and then skip the blocks calls when you have non-null render data in the array. Hard to say which way will be more prevalent.

I absolutely think that, but the only case where you'd get non-BlockState data out of a Block is neighboring blocks in terrain...

...which, while rare in terms of mod author count, is probably going to have a lot of blocks involved.

I absolutely think that, but the only case where you'd get non-BlockState data out of a Block is neighboring blocks in terrain...

Actually, for dynamic terrain I implement a separate client-side cache mechanism to hold partial computations by block position. It listens for world updates and clears the cache as needed. It also does cache the final result to avoid recomputes. It's complex but avoids a lot of state lookups.

Blocks in that case will query the cache first (based on world and block pos) and then only recompute if necessary.

Earlier I used Block entities that were entirely transient to cache some of the lookups, but there's really no such thing as a lightweight Block Entity when it comes to network packets, sooo... specialized cache.

Actually, for dynamic terrain I implement a separate client-side cache mechanism to hold partial computations by block position. It listens for world updates and clears the cache as needed. It also does cache the final result to avoid recomputes. It's complex but avoids a lot of state lookups.

Yes, you could pass this as a render cache object too. Nice, but still means we should probably allow Blocks and BlockEntities both to provide render data objects.

Made some changes, in particular:

• after a bit of discussion, we decided that the few mods which would benefit from an explicit Consumer are probably better off adding specific support for another render pipeline - besides, it's an addition, so we can re-look into it later;
• Block's render data getter has been moved to the Model. Mojang assumes all light value information to be thread-safe; in addition, BlockStates, FluidStates and block entity render data is thread-safe - as such, we want to maximally promote performing heavy render data computation in the chunk render worker threads, off the main thread. (Not to mention, it removes an iteration from ChunkCache)

Block's render data getter has been moved to the Model. Mojang assumes all light value information to be thread-safe; in addition, BlockStates, FluidStates and block entity render data is thread-safe - as such, we want to maximally promote performing heavy render data computation in the chunk render worker threads, off the main thread.

When does the model do this, and where does it store the resulting data between calls to getQuads for each block face? We should avoid making it retrieve state on each call?

When does the model do this, and where does it store the resulting data between calls to getQuads for each block face? We should avoid making it retrieve state on each call?

It does it at the beginning of model tesselation, so once per block - this has not changed. This approach has two key advantages:

• as non-block-entity data is handled mostly thread-safely, we can perform these computations multithreaded and save CPU time on the main thread,
• we don't need to iterate through all Blocks, merely ones which have a dynamic model.

There is a disadvantage, however - it's now done once per rendering layer, so up to four times if your block renders on multiple layers (which is uncommon, but possible with a mixin - not by default, though!). Something to think about.

I see it now. Makes sense.

The latest rev looks ok to me besides missing Javadoc.

for DynamicBakedModel.getRenderData's javadoc:

• please document that it is unspecified whether it may get called for every layer or only once - it being called for every layer is just an implementation artefact
• add information about the existence/combined use of BlockEntity.getRenderData

The handling of RenderDataProvidingBlockEntity should be changed in this or a follow-up PR to maintain a separate list on the (render?)chunk that tracks implementing classes to avoid iterating all entities. Addition and removal happens in line with the chunk's regular be map.

I think follow-up is sufficient for this.

FWIW I did investigate this for #55 and it doesn't appear to be a straightforward patch. When the cache view is being created it doesn't have a reference to the ChunkRenderer.

Would be logical to handle this in method_3652 when block entities are already being reconciled. Unfortunately that happens on the render threads so it doesn't work for our purpose.

I looked briefly at feasibility of handling in WorldChunk but wary of going there.

Deferring for now.

For extra flexibility and performance the following changes should be considered:

Materials

Replace the concept of render layers with render materials, where a material choice defines the required vertex format and how the quads are to be rendered.

All vertex formats have to keep the first 24 bytes identical to vanilla for easy analysis or transformation, i.e. position, color and texture. The vertex format is defined by and consistent across the material.

The material is also responsible for processing the models' vertex data. It receives the output of getQuads, processes it internally to apply light, tint or whatever it desires and normally outputs to the vanilla vertex data buffers or its own. Vanilla's layer materials delegate to the existing render methods with the appropriate buffer. The format of non-vanilla vertex data buffers is unspecified.

DynamicBakedModel.getQuads has to be called exclusively for the materials returned by DynamicBakedModel.getMaterials, which defaults to the material corresponding to Block.getRenderLayer.

Support for providing custom materials can be added at a later time. The 1:1 relationship between materials and layers makes it trivial to adjust the PR for vanilla materials only.

Examples:

vanilla translucent material: vertex format pos+color+uv+padding, render with alpha blending
emissive translucent material: vertex format pos+color+uv+emissivity, render with alpha blending and the block light map clamped

Allocation prevention

Allocations for the List and BakedQuad instances returned by getQuads in the non-static case can be avoided by caching the objects thread local. For this to work the documented has to specify that the returned list and quads are only valid until the next getQuads invocation on any model on the same thread.

Allowing caches

There needs to be a way to determine what getQuads does, with conservative defaults. Properties of interest are:

• Are the returned list + quads constant? (e.g. not reused for other contents in a TL cache or similar, otherwise a cache has to copy)
• Does the return value depend on renderData?
• .. or state?
• .. or random?

RenderDataType

The generic type variable is extra noise and technically incorrect. In Java the caller defines its value, not the callee.

Render Types (for later)

In addition to dynamic models, custom render types besides normal model, liquids and no-op can provide a cleaner and faster hook for rendering outside the model system. Their implementation is responsible for the entire processing chain between state+world+pos and the output vertex data buffers.

PR is up, but want to respond directly to the points Player made, because it may not be immediately clear how the new PR addresses them...

Materials
Materials are in, and they do replace BlockRenderLayer. BlockRenderLayer remained useful as an attribute of materials to specify texture blending - I couldn't think of any other useful combinations of cutout/translucent/LOD and people already understand how those four work.

Materials do imply vertex format, but in the latest PR the implementation of material is unconstrained. The important interface is the Material Builder, which defined the standard feature set of what can be specified in a material: emissive, overlays and blending.

getQuads()
For static models, I see the utility of being able to control the order in which materials are piped to vertex buffer, and how. Ultimately this was resolved by delegating the static version of the model implementation itself.

For dynamic models, some more explicit method for vertex transfer must be exposed, but I don't want to expose vertex formats because we will never all agree on what they should be and if they aren't standard, we're back to analyzing formats on the fly.

To handle dynamic vertex data, I abstracted a subset of BufferBuilder functionality and again left the rest up to the implementation.

In the dynamic case, the model is not required to pre-sort by material. I trust the ModelRenderer implementers to handle material sorting efficiently more than I trust everyone else who might ever create a dynamic vertex producer.

Allocation / Caches
Agree with your comments here, but ends up not mattering in the current PR.

Render Types
Still a very clean hook but not needed for the current PR. Using it or not will up to the implementation. There are some trade-offs in that it creates multiple logic paths for models that should be rendered with the same lighting, which in turn depends on some world state that could be reused for both. Don't think we should try to constrain that choice.