Add Sky Rendering Events#5328
Conversation
…g what it is and from what mod
|
Seems good to me, but I think it might be useful to add an extraction callback like |
Done |
CallMeEchoCodes
left a comment
There was a problem hiding this comment.
LGTM, Although unsure about impl details with extraction
| private void setupContext(ClientLevel level, float partialTicks, Camera camera, SkyRenderState skyRenderState, CallbackInfo ci) { | ||
| celestialContext.prepare((SkyRenderer) (Object) this, skyRenderState); | ||
| final CameraRenderState cameraRenderState = new CameraRenderState(); | ||
| camera.extractRenderState(cameraRenderState, partialTicks); |
There was a problem hiding this comment.
Iffy on extracting the camera a second time (although I don't think it will cause any issues)
Is there some way to pass this down properly?
There was a problem hiding this comment.
At a glance it should be possible to put a ScopedValue or ThreadLocal in LevelRenderer::addSkyPass, I'd probably wrap that whole method and add a ScopedValue with the CameraRenderState
There was a problem hiding this comment.
Still approved as this can be changed later without breaking api
There was a problem hiding this comment.
This code is in SkyRenderer, how would you want me to pass it there?
There was a problem hiding this comment.
I don't think this needs to be here. This would be easier LevelRendererMixin. Also, SkyRenderContext could extend AbstractLevelRenderContext, and then you only need to provide skyRenderer. This also means that we don't need a SkyRenderContextImpl and instead just add it to the existing LevelRenderContextImpl.
So basically, add skyRenderer to LevelRenderContextImpl, make it implement SkyRenderContext extends AbstractLevelRenderContext, and set levelRenderContext.skyRenderer in beforeRender.
|
|
||
| @Inject(method = "extractRenderState", at = @At("TAIL")) | ||
| private void afterExtractSky(ClientLevel level, float partialTicks, Camera camera, SkyRenderState state, CallbackInfo ci) { | ||
| SkyRenderEvents.END_EXTRACTION.invoker().execute(new SkyExtractionContextImpl(level, camera, state, partialTicks)); |
There was a problem hiding this comment.
Should they be "render" events if they happen during extraction?
There was a problem hiding this comment.
The LevelRenderEvents has END_EXTRACTION so I matched that
| final boolean cancelled = SkyRenderEvents.PRE_SKY.invoker().execute(skyRenderContext); | ||
| if (!cancelled) { |
There was a problem hiding this comment.
Doesnt build, wont pass checkstyle (and many other places)
| @ApiStatus.NonExtendable | ||
| public interface CelestialRenderContext extends SkyRenderContext { |
Client crashes as well. |
Hmm will check out |
kevinthegreat1
left a comment
There was a problem hiding this comment.
While I don't think many of the events are needed, we are still discussing that. Meanwhile, I have commented on some implementation things that I think should be done regardless of which approach we take in the end.
| private void setupContext(ClientLevel level, float partialTicks, Camera camera, SkyRenderState skyRenderState, CallbackInfo ci) { | ||
| celestialContext.prepare((SkyRenderer) (Object) this, skyRenderState); | ||
| final CameraRenderState cameraRenderState = new CameraRenderState(); | ||
| camera.extractRenderState(cameraRenderState, partialTicks); |
There was a problem hiding this comment.
I don't think this needs to be here. This would be easier LevelRendererMixin. Also, SkyRenderContext could extend AbstractLevelRenderContext, and then you only need to provide skyRenderer. This also means that we don't need a SkyRenderContextImpl and instead just add it to the existing LevelRenderContextImpl.
So basically, add skyRenderer to LevelRenderContextImpl, make it implement SkyRenderContext extends AbstractLevelRenderContext, and set levelRenderContext.skyRenderer in beforeRender.
| * <p>To attach modded data to vanilla render states, see {@link net.fabricmc.fabric.api.client.rendering.v1.FabricRenderState FabricRenderState}. | ||
| * Only attach the minimum data needed for rendering. Do not attach objects that are not thread-safe such as {@link net.minecraft.client.multiplayer.ClientLevel}. | ||
| */ | ||
| public static final Event<EndExtraction> END_EXTRACTION = EventFactory.createArrayBacked(EndExtraction.class, callbacks -> context -> { |
There was a problem hiding this comment.
This is not needed, use LevelRenderEvents.END_EXTRACTION instead. We intentionally only added one extraction event.
FlashyReese
left a comment
There was a problem hiding this comment.
I think this is a good direction. Proper sky render events would be useful for mods that modify vanilla sky rendering and mods that fully replace or layer custom skies.
My main concern is that the API currently feels split between vanilla sky element interception and a broader custom sky render hook. For full custom sky renderers, the important part is having one clear sky render phase with documented render-state guarantees, otherwise mods may still need their own LevelRenderer/SkyRenderer mixins.
The things I think should be clarified before this becomes API:
- should cancellable
PREevents still call every listener before returning the final cancel result? - should all sky events share one context source/lifecycle?
- what render state is guaranteed during
PRE_SKY? - should
PRE_CUSTOM_ELEMENT/POST_CUSTOM_ELEMENTbe typed, or left out for now?
I am mostly looking at this from the perspective of custom sky rendering where mods need to stack or replace skies, not only cancel vanilla sun/moon/stars.
| */ | ||
| public static final Event<PreSky> PRE_SKY = EventFactory.createArrayBacked(PreSky.class, callbacks -> context -> { | ||
| for (final PreSky callback : callbacks) { | ||
| if (callback.execute(context)) { |
There was a problem hiding this comment.
Should this call every callback and OR the cancellation result instead of returning immediately?
If one mod cancels here, later listeners never see the event at all. That seems rough for compatibility between multiple sky mods.
| import net.minecraft.client.renderer.state.level.SkyRenderState; | ||
|
|
||
| @ApiStatus.NonExtendable | ||
| public interface SkyRenderContext { |
There was a problem hiding this comment.
Should this extend AbstractLevelRenderContext?
Sky rendering is still part of the level render frame, so access to levelRenderer/gameRenderer/levelState seems useful and keeps this consistent with the rest of the rendering API.
| /** | ||
| * Called at the start of the "addSkyPass" lambda. | ||
| */ | ||
| public static final Event<PreSky> PRE_SKY = EventFactory.createArrayBacked(PreSky.class, callbacks -> context -> { |
There was a problem hiding this comment.
This seems like the event custom sky renderers would depend on the most.
Can the javadocs define what render state is guaranteed here? Mainly fog state, matrix/projection expectations, and whether returning true skips all vanilla sky rendering.
|
|
||
| @FunctionalInterface | ||
| public interface PreCustomElement { | ||
| boolean execute(Identifier key, Object context); |
There was a problem hiding this comment.
I am not sure about Object context here.
If Fabric does not invoke this itself and the context is untyped, it seems hard for mods to rely on. I think this should either be typed more strongly or left out for now.
This will be my first contribution to Fabric API. Had to rework some events from my original to work w/ fabric-api's ecosystem and how it handles events.
Events for each portion of the sky rendering, with corresponding important data accessible during those phases. Allowing cancelling of rendering such as the stars/sun/moon.
Also adds a custom element even in which although unused by FAPI, is intended for mod developers to invoke when adding custom elements to the sky when rendering allowing other mods to intercept them and do as please.
Progress:
Everything is implemented, only needs testing & javadoc work now.