Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create AddSectionGeometryEvent #642

Merged
merged 8 commits into from
Feb 25, 2024

Conversation

malte0811
Copy link
Contributor

This event can be used to implement features that add static geometry to the world based on things other than blocks. See here for an example usage for Immersive Engineering wires (which is my usecase for this event).

The performance impact is one event fired on the main thread for each chunk section for which a rebuild task is started, which should be acceptable, plus whatever overhead mods add in their event handlers and custom renderers.

Some points brought up by @Technici4n in an earlier discussion:

  • Collection of data on the main thread for thread-safety
    • The event is fired on the main thread and the actual renderer is then added as a callback to be invoked on the meshing thread. Typical usage would be to collect the data in the event handler and then capture it in the lambda.
  • How will the AO work for the extra quads?
    • SectionRenderingContext::getQuadLighter can be used to obtain both AO and non-AO lighters as required
  • The system should ideally be usable for other rendering systems (e.g. guidebook previews) without too much effort
    • The main obstacle for guidebook usage is the fact that a full Level is required. I think this is reasonable since
      a) it is possible with reasonable effort to create a fake Level instance and
      b) most usecases will need access to data beyond what the "easier" interfaces of Level can provide (e.g. data attachments).

CC @embeddedt, since we had some discussions about implementing this in Embeddium: I know you do not have all the objects needed to construct a SectionRenderingContext directly. However I did not want to use a Function here when there is no direct motivation for doing so in vanilla/NeoForge. My suggestion would be a Mixin that adds whatever fields you need to the context (set to null in the constructor), and chooses between a custom implementation of getOrCreateBuffer and the default one based on which fields are set.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@embeddedt
Copy link
Member

However I did not want to use a Function here when there is no direct motivation for doing so in vanilla/NeoForge.

I still think it would be cleaner to use a Function since it allows hiding the implementation details of getOrCreateBuffer at the location where the event is fired, instead of dealing with that low-level detail of vanilla in the event class.

@Technici4n
Copy link
Member

It would be nice to have a test mod as well :)

@Technici4n Technici4n self-assigned this Feb 18, 2024
@FiniteReality
Copy link
Member

FiniteReality commented Feb 19, 2024

There have been some previous attempts at systems for adding custom chunk geometry previously:

However, for various reasons they've all been held up and have eventually gone stale.

It would be nice to see some of those changes included here too, such as the ability to add custom buffers to the SectionBufferBuilderPack, because as it stands, it appears that calling SectionRenderingConstext.getOrCreateBuffer(myCustomRenderType) will actually throw a NPE, as SectionBufferBuilderPack.builder() returns from a static map which does not include myCustomRenderType

@Technici4n
Copy link
Member

Adding buffers for chunk meshing is completely out of scope for this PR.

@FiniteReality
Copy link
Member

Adding buffers for chunk meshing is completely out of scope for this PR.

I disagree entirely. This API is about adding custom geometry when meshing; it is only natural that we consider the buffers where that geometry is stored.

@FiniteReality
Copy link
Member

FiniteReality commented Feb 19, 2024

Even disregarding the adding buffers issue, it is incredibly poor API design for users to be able to pass custom render types to the APIs proposed here and have them throw a NPE, when it clearly looks like it should be supported based on the name of getOrCreateBuffer. This method implies that custom buffers are supported implicitly, which is not the case. If this is intentional, it should instead be broken into methods such as getTripwireBuffer() or getCutoutBuffer().

@embeddedt
Copy link
Member

Alternatively, we can call it getChunkRenderTypeBuffer, and specify in the javadoc that only the render types listed in RenderType.chunkBufferLayers() (if I remember the name correctly) are valid inputs.

@Technici4n
Copy link
Member

Adding layers is a whole new level of complexity that should not be tackled in this PR. However I would indeed encourage a way to prevent errors where a user thinks that any buffer can be used.

@Shadows-of-Fire Shadows-of-Fire added enhancement New (or improvement to existing) feature or request 1.20 Targeted at Minecraft 1.20 rendering Related to rendering labels Feb 19, 2024
@malte0811
Copy link
Contributor Author

These commits should address all points raised so far. Additionally they fix the event for sections that are "far away" from sections containing blocks.

@marchermans marchermans merged commit 58cd583 into neoforged:1.20.x Feb 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Targeted at Minecraft 1.20 enhancement New (or improvement to existing) feature or request rendering Related to rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants