Sodium

Sodium

35M Downloads

Sodium shouldn't try to cast VertexConsumer to VertexBufferWriter without checks

Igrium opened this issue ยท 6 comments

commented

At some point within the Sodium code, Minecraft's VertexConsumer is cast to a VertexBufferWriter without any instanceof checks. Presumably, this is handled for vanilla code using a mixin, but it crashes any third-party mods that, for whatever reason, need to implement their own VertexConsumer (see: Igrium's Replay Exporter).

Due to the nature of the issue, the only way for third-party mods to fix this on their end is to hard-depend on Sodium and implement VertexBufferWriter themselves, which leads to a dependency hell and is simply not viable. Is there a way to code a check and make whatever optimization Sodium is trying to enact soft-fail if the cast fails?

(Relevant crash log: https://mclo.gs/YnCg400)

commented

Relevant issue: #1620

commented

Realistically, we're stuck between implementing two versions of every code path that touches a VertexConsumer (which isn't happening) or we have to abort our code paths when we encounter something weird (which hurts performance and complicates code greatly.) The latter also don't work in all cases, since many of Sodium's internals are only written against the VertexBufferWriter implementation.

commented

I don't want to break mods, but I seriously can't see a way to make the terrible VertexConsumer interface any better. And the performance uplift that users get with our fast-path is significant, sometimes 50-75% faster for immediate mode rendering.

The best we can do is try to expose a public API, and allow mods to implement the VertexBufferWriter interface without needing to hard-depend on the rest of Sodium. We've already prepared that for the next release of Sodium... it's just a matter of making sure the API is any good before we bother other mod developers with it.

commented

That would probably work! How would one actually do that in their code?

commented

A pull request is currently open which fixes this problem: #1842

commented

Fixed in merge commit 5b57419.