Not a fan of the cutesy "tailor" names. Naming projects like that is one thing, but the code should be kept readable/easily understandable, IMO.

Yeah, it's a straw man. EnhancedBakedQuad was too unwieldy for me. Any suggestions? FancyBakedQuad? FabricBakedQuad?

I think EnhancedBakedQuad is fine, but FabricBakedQuad is also good.

Maybe use Dynamic as the prefix, since the purpose of this seems to be to provide more dynamic models.

I don't like the FabricBakedQuad getter being separate from the BakedQuad getter: I think FabricBakedQuads should gracefully fall back to regular quads (by, say, having a null int[] field containing the original vertex data populated on demand), and then - now that the RenderData gathering is similar - I think we can just treat this PR as an extension of #45, no?

On the other hand, checking every BakedQuad for it being a Fabric one or not could get pricey...

Trying to avoid the per-quad check, yes, and also the fabric getter is all sides at once. But mostly, the fabric getter needs additional parameters, which forces it to be different.

The vertex format standards will require the first 28 elements of vertex data to match the standard Minecraft block format - with a few situational exceptions. This ensures the fallback getQuads is trivial to implement when it matters and requires no copying or transformation of the vertex data. I hope to provide a default implementation of that interface method.

That said, I'll consider ways to make the two more harmonious - maybe adding back the side parameter, which would certainly help with the default implementation be more efficient.

**edit: I don't see any way to provide a default implementation of vanilla getQuads that doesn't constrain implementations in bad ways. A key benefit to single pass is the model doesn't have to store model state between sides/layers. The vanilla get quads method forces a pass per side, plus general quads. And I also can't know what model state assumptions to use in the default case.

I think standardizing the vertex format is the most impactful thing, because having to transform or repack vertices would kill performance. If I can show in the reference implementation that implementing the vanilla getQuads is a low bar, I hope that is sufficient.**

This is indeed ending up very close to #45 in approach, which gives me confidence we're on the right track, because I came to the same place by a somewhat different route. I still don't like that Block Entities have to know about their models but I also see no good way around it.

There are some small but significant bits still to be added and documented, but this PR by itself will not make any changes to model baking, lighting or rendering. It will simply define standards and create minimal hooks for those things to be done in an interoperable way. To use any extended features (emissive, for example) will require an implementation of the extended model/quad interfaces and an active rendering plugin.

Should be ready for thorough going-over in a couple days, but then I'd like to create minimal reference implementations, separate from the PR, and demonstrate some common use cases and run it through the profiler to be sure it holds up.

We then might want to cherry-pick some of that reference implementation into a separate PR to make things easier for mod authors and provide some basic "out-of-box" capabilities, but that's to be seen.

Lastly this doesn't fully address block entity rendering and it doesn't address item rendering at all. I'd like to come back to those after this is wrapped up.

One other thing regarding getFabricBakedQuads. Because we're requesting a List instance, the interface contract needs to guarantee to the implementer the list reference won't be retained. This allows implementers to re-use list instances, which will be important to reduce allocation overhead for mods doing dynamic mesh generation/baking at render time.

I would prefer to enforce this guarantee in code vs spec, but am trying not to do anything too exotic.

What I'm saying is - I'd like the "dynamic baked model" and "advanced-quad baked model" patches to be separate in the sense of not having to do one to get the other, as well as greater/easier cooperation for cross-mod-loader compat in the future, possibly. Much easier to feed in a RenderDataType object than it is to support a whole new quad type.

Also, keep in mind int[] being of size 28 is not always a guarantee - for instance, OptiFine's shader patches will cause it to be of size 56.

All of this is why I'd ideally like #55, if it ends up merged, to build on #45, and - by extension - for #45 to be merged first.

I'm still iffy about this adding features to the engine versus being hooks/interop - I'm fearing setting a precedent in which Fabric's API module stops being that. But I understand the needs. Perhaps it would be better to create another module under the "official Fabric" umbrella which provides advanced rendering complete with basic implementations?

With the changes just pushed, this is now far less invasive that #45. I could not extend #45 without removing substantial parts of it.

As it stands, this no longer makes any changes to vertex lighting or buffering. There are no patches to BlockModelRenderer. Instead, it simply defines standards for implementing a RenderPlugin and for enhanced models to provide vertex data to the plugin. Existing baked models and quads will support any render plug via purely additive extensions.

After going back and forth over all the ways to implement hook for model lighting and buffering, I believe the best and most performant will involve replacing large swaths of BlockModelRenderer, WorldRenderer, etc. That obviously has no place in the Fabric API, and if we instead try to insert more conservative hooks we are probably wasting our time and possibly making life more complicated for future plugin authors who will then need to reproduce or work around those hooks in their own patches.

Regarding the vertex formats: the defined formats are for model vertex data being sent to a rendering plug in. This is the key to interoperability for render plugin and models. It also has the nice side effect of insulating model authors from future changes to the rendering pipeline by Mojang, because model vertex formats become independent.

The first-28 convention is essential for compatibility when a rendering plug-in is absent, and it also provides performance and compatibility benefits for plug in implementors. While this spec will offer guidance on how vertex buffers ought to be populated for benefit of shader compatibility across render plugins, it does not constrain it. A render plug-in author can use whatever vertex formats are appropriate for their implementation.

A bare-bones reference implementation of a render plug-in may be useful as a Fabric-adjacent project, so that mod authors can be guaranteed at least some minimal level of features. I intend to create one to validate the approach, so we could evaluate that when I have something to show. A little down the road it may also be useful to have a project devoted to developing standards for render-side vertex formats, shader-libraries, etc. for people who want to implement a render plug in or employ custom shaders.

This is close to done now. I want to add more docs explaining vertex formats and expectations for a render plug in, and at least stub in how BlockEntityRenderers and Item Rendering will work to make sure it hangs together.

Yes, I mean - since Fabric only provides the standard vertex formats, there is no reason why it should be decisive over the flags.

This is slowly getting to a point where I'm pleased with it, but I need to get Player to look at the performance and assess that...

Edit: not actually rendering yet. My build setup was borked and I was too tired to notice.

Needs a little more bit twiddling to render properly. Still very pleased with how API works as an abstraction layer.

first plugin screenshot

Very basic plug-in rendering now. Flat lighting only, no extra features. Was straightforward to implement - models and rendering are nicely decoupled. Performance seems good, subjectively. Won't do benchmarks until smooth lighting works and have confirmed visual match.

Pressed for time so code isn't pretty but at least there isn't much of it. Can be seen here: link to source

I think I can say that I like the direction in which this is going. Just waiting for some performance analysis/"roasts" from Player now.

FIXED: Made FabricBakedQuad.getRenderLayerFlags() unambiguous so plug-ins can distinguish
quads that use block render layer vs quads that specify their own render layer(s).

We've discussed some things about this PR recently on IRC.

The way you structure dynamic models means you move side/layer decisions to the rendered model. This effectively means that the models have to be "smarter", by force - but it also prevents easy re-use of their data in other contexts (say, as parts of a bigger model); now, while I see no issue to having the layer be part of quad information (and it has advantages!), I have to create a fake BlockView to be sure that I have dumped all the data from the model. One solution could be to create an "inbetween" model interface: a marker interface which says: this model's getQuads() outputs FabricBakedQuads and opts into the newer pipelines, but does not use any dynamic generation. Maybe there's others.

Also, something I've noticed: FabricBakedQuad having methods named exactly as their vanilla counterparts without extending can cause issues with obfuscation, though I haven't tested this - have you?

The way you structure dynamic models means you move side/layer decisions to the rendered model. This effectively means that the models have to be "smarter", by force - but it also prevents easy re-use of their data in other contexts (say, as parts of a bigger model); now, while I see no issue to having the layer be part of quad information (and it has advantages!), I have to create a fake BlockView to be sure that I have dumped all the data from the model. One solution could be to create an "inbetween" model interface: a marker interface which says: this model's getQuads() outputs FabricBakedQuads and opts into the newer pipelines, but does not use any dynamic generation. Maybe there's others.

To make sure I understand, the concern is having a way to extract a complete model representation without a block view, because a call to getQuads doesn't necessarily return FabricBakedQuads, and a call to produceFabricBakedQuads() requires a BlockView if I want to get block quads.

That's a legitimate concern. I had been assuming consumers who wrap the model would do so in each context (block, item, FRBE) and so would already have the necessary parameters in hand, but maybe they won't. Models get used in other ways.

One thing I'd like to preserve is automatic and reliable forward compatibility for vanilla baked models and quads. It makes life easier for plug-in authors or really anyone who wants to consume the new interfaces. They only need to write one handler.

My initial thought is to split produceFabricBakedQuads() into three different methods:

// for block renders
produceBlockQuads(ModelBlockView blockView, BlockState state, BlockPos pos, Random random, long seed, Consumer<FabricBakedQuad> consumer);

// for fast-rendering block entities
produceBlockEntityQuads(@Nullable Object RenderData, BlockState state, BlockPos pos, Consumer<FabricBakedQuad> consumer);

// for generic block quads and item models
produceAllQuads(@Nullable Object modelState, Consumer<FabricBakedQuad> consumer);

This makes it more explicit what is being requested from the model without the need to test parameters. And a default implementation for vanilla models remains feasible. It also fixes a defect for FBER I hadn't spotted yet - it has no way to receive its render data. Default implementations remain viable.

I don't like that produceAllQuads() remains ambiguous - the vertex format depends on the type of model you are calling. If you have a block model you'll get block quads. If you have an item model (via getItemPropertyOverrides()) you'll get item quads. But it seems useful to remain consistent with how getQuads() currently works. And if we did add separate methods for block and item quads, are we then requiring models to output both on demand? And if not, why bother?

Consumers who get a dump of generic block quads via produceAllQuads() with a null modelState will get the "default" quads - there is nothing else for the model to go on. That's the same behavior as getQuads() today. The optional ModelState parameter is there for mod authors who may have use of it.

For example, a possible model implementation pattern is to implement quad output in produceAllQuads and then make the implementations of produceBlockQuads and produceBlockEntityQuads derive a model state which they then pass to produceAllQuads. But we would not be forcing that - it doesn't make sense for all models.

Did I understand the concern correctly? Thoughts?

Also, something I've noticed: FabricBakedQuad having methods named exactly as their vanilla counterparts without extending can cause issues with obfuscation, though I haven't tested this - have you?

Good catch. Obvious in retrospect but I didn't spot and have not tested. It does seem certain to cause problems, at least for the default implementation in BakedQuad. I recall Mixin has annotations to help finesse obfuscation edge cases but I haven't used them much. I'll look into this but my bias is towards simple and reliable - will probably be best to give the methods different names.

edit: voldemapping

To make sure I understand, the concern is having a way to extract a complete model representation without a block view, because a call to getQuads doesn't necessarily return FabricBakedQuads, and a call to produceFabricBakedQuads() requires a BlockView if I want to get block quads.

Yes, but also a way to reproduce a complete model representation for, say, doing your own caching. What I mean is - with getQuads(), you can get the quads for the center, then the quads for each culling side, and you can make culling decisions yourself; with produceFabricBakedQuads(), you have to trust in the model every time.

What I'm really trying to get at here is that our model API as it stands is good for truly dynamic models, but is bad for the middle case: a static model wanting to make use of enhanced rendering features, and then dynamic models wanting to build on top of that. Take the SimpleLogic gate model, which is built from models of redstone torches translated accordingly. My solution was to, well, add some documented/supported measure of creating a static "dumb/List" model which can still benefit from Fabric's extensions. Another thing to see would be if, instead of Block.shouldDrawSide, we could expose some helper methods in the ModelBlockView (that could then be overridden!).

On a side-note, indexing BlockEntities by BlockStates (-> BakedModels) doesn't seem like a great idea performance-wise, but I'd have to check if both are easily available at rendering time.

...with getQuads(), you can get the quads for the center, then the quads for each culling side, and you can make culling decisions yourself; with produceFabricBakedQuads(), you have to trust in the model every time.

I understand. I can add methods that accept a Direction parameter to emulate how getQuads() works. I'd prefer to keep the Consumer<> pattern vs. forcing the model to instantiate lists, but that's negotiable.

I'd like to keep the existing all-at-once methods for use by plug-ins, but I will add default implementations for them based on the per-side versions - it's trivial to do.

One of my goals here is to make it easy for mod authors with high-performance needs to use a single int[] array as the internal model representation. And then let plug-ins fully exploit that when available. This has significant benefits for locality of reference and allocation overhead, but I agree many, probably most, won't need it.

Those who do want to optimize can simply override the all-at-once producer methods and satisfy the per-side version by sorting the quads in the array by face and then storing some offsets to know where each face starts. The offsets can even be packed into the same array. They will need to do something similar anyway to satisfy the BakedQuad.getQuads() method for compatibility.

Acceptable?

On a side-note, indexing BlockEntities by BlockStates (-> BakedModels) doesn't seem like a great idea performance-wise, but I'd have to check if both are easily available at rendering time.

Could be. I assumed but didn't require that plug-ins would pre-fetch and cache the models in some performant way before it ever got to that point. But the whole FastRenderingBlockEntity thing isn't fully "baked" yet (ha!). For example, I think per-frame, animated renderers may also need to get a fractional tick value sent to them. But I really won't know for sure until I run some experiments.

My main intention with FBRE was to show that some kind of fast block entity rendering scheme is in-scope and start getting feedback on it. If we arrive at a point where the rest is ready but the FRBE isn't we can just push it to a follow-on PR. IRRC it's only one interface at this point.

I'd like to keep the existing all-at-once methods for use by plug-ins, but I will add default implementations for them based on the per-side versions - it's trivial to do.

I'm saying have two interfaces: FabricBakedModel (preserves getQuads, but allows additional FabricBakedQuad features etc.) and DynamicFabricBakedModel (consumer pattern + renderdata support).

I'm saying have two interfaces: FabricBakedModel (preserves getQuads) and DynamicFabricBakedModel (consumer pattern + renderdata support).

Ah, yes, that seems promising. Ends up being equivalent for the plug-in but easier to work with for mod authors who just want fancy renders on a static model. I'll have a revision up in the next couple days.

Separate but related topic:

Would you consider making emissive render for single-layer quads a minimum standard? That, and being able to specify multiple block render layers in the same model?

The render layer attribute is already there - I'd just need to document that plug-ins are required to honor it for single-layer quads.

For emissive, I'd handle by stating that plug-ins must honor light maps in the BLOCK format as minimums. Right now it says they may.

Block lightmaps won't work with item renders - those would require a plug-in specific extension. If we do want emissive item renders as part of a minimum standard, we'd need to add an extended vertex format that includes both normals and lightmap and also require plug-ins to recognize it. I'd be OK with that if you are.

I'll note here that adding a standard vertex format that includes both normals and lightmap, and requiring plug-ins to recognize it, has another big benefit for model authors who elect to use it: they can return the same vertex data for both item and block renders. They don't need to produce separate formats (except for vanilla getQuads). But it does take us to the edge, if not outside, of what I understand to be your intended scope for the core Fabric API.

These minimums put a little more burden on plug-in authors but they were probably going to implement those features anyway and it would give mod authors the assurance of some baseline capabilities that satisfy the most common use cases.

Thoughts?

Looks like this will be fully superseded by #65, so closing out to prevent any confusion or waste of time for reviewers.