GTParticle/bloom effect refactor and cleanup #2073
Conversation
|
In-game testing seems fine, except for energy laser particles disappearing after exiting and rejoining a world. |
| for (GTParticle particle : newParticleQueue) { | ||
| var queue = particle.shouldDisableDepth() ? depthDisabledParticles : depthEnabledParticles; | ||
|
|
||
| ArrayDeque<GTParticle> particles = queue.computeIfAbsent(particle.getRenderSetup(), setup -> new ArrayDeque<>()); |
There was a problem hiding this comment.
Since particle.getRenderSetup() is nullable, we may we wish to skip this operation altogether. Associating a deque for a null key sounds like cause for potential issues here. If using null this way is intentional, I think some sort of explicit dummy IRenderSetup would be more appropriate.
There was a problem hiding this comment.
Nullable render setup instance is an explicit decision here, because I found the notion of null render setups more straightforward - "no render setup" directly correlates to "no render setup instance (null)", while using nonnull representation you have to say there should be something even if the thing that object represents is just nothing. Plus it avoids pitfall of users just making empty impl for the interface because returning null crashes the game with NPE, breaking render setup's equality contract in the process.
The inner part of the API (the part of code this conversation is taking place) also uses nullable keys, because (1) the collection lib gracefully supports it (fuck you stdlib), (2) it's (probably) (slightly) faster and (3) there's no need to make null-object representation if base API does not uses it.
While the key nullability is undocumented, the render setup is used in one place, and the collection is only internal, so I think there's no big risk there. I personally would have liked nullable annotation in the type (like Map<@Nullable IRenderSetup, ArrayDeque<GTParticle>>), but I couldn't do that since I was using findbugs nullable annotations, which cannot target types.
Still I probably have to document the nullability lol
| public int getValue() { | ||
| return switch (this) { | ||
| case GAUSSIAN -> 0; | ||
| case UNITY -> 1; | ||
| case UNREAL -> 2; | ||
| case DISABLED -> -1; | ||
| }; | ||
| } | ||
|
|
||
| @Nonnull | ||
| public static BloomType fromValue(int value) { |
There was a problem hiding this comment.
Instead of these two methods, I think we should use the enum in the config directly. Forge does natively support this and does also display a list of valid enum constants to choose in the config file.
# Conflicts: # src/main/java/gregtech/api/cover/CoverBehavior.java # src/main/java/gregtech/common/metatileentities/multi/electric/MetaTileEntityAssemblyLine.java
src/main/java/gregtech/common/metatileentities/multi/electric/MetaTileEntityFusionReactor.java
Outdated
Show resolved
Hide resolved
What
This PR is
yet another "internal refactor" PR that murders backwards compatibility for the sake of delusional concepts such as "cleaner code" and nothing elsea refactor and cleanup for small part of client code, namelyGTParticleand bloom effect code.Implementation Details
GTParticlerefactorAmong 5
GTParticleimplementations only 3 of them are used in base mod. Not to mention most of the functionality contained inGTParticleare not utilized. Since the 3 implementations used in base mod are all non-universal implementations for only one use case each. I've decided to refactor allGTParticleimplementations to reflect these use cases.GTParticlebase class provides by default.GTParticleno longer directly extends vanillaParticle, which removes things like particle movements, colors, particle ages etc. Also the weird atlas animation thing was removed from GTParticle.GTTexturedParticleandGTTexturedShaderParticle.IGTParticleHandlerto base interfaceICustomRenderFast, which got renamed toRenderSetupto reflect the functionality of the type better.Parameter of
GTParticleManager#addEffectwas changed fromaddEffect(GTParticle...)toaddEffect(GTParticle).GTParticleManagerhas additional behavior change compared to previous iteration; on particle draw, particles with depth mask disabled now gets rendered first instead of last. This behavior is actually in line with vanilla particle manager, and since I don't see reasoning behind this particular change, I just applied vanilla logic.Bloom effect refactor
Most of the changes are naming corrections and code restructurings. One major change, however, is the removal of
requestCustomBloomin favor of the new methodscheduleBloomRender.IBloomRenderFastwas also migrated to base classICustomRenderFast, which got renamed toRenderSetup.However this approach is still massively inefficient, because each custom bloom render still has to be registered each tick. This could produce nontrivial amount of heap pollution if enough renders get scheduled, but for now the performance impact shouldn't be apparent at least on base mod since this feature is currently being used on whopping 2 places.
Some fields and methods were preserved for compatibility reasons, namely
BloomEffectUtil#BLOOMandBloomEffectUtil#getRealBloomLayer. The latter was renamed to avoid connotations of the returned object being the "real" bloom layer, which isn't true.Others (TM)
Changed names of
Easesenum because I couldn't stand it. The old names are provided with static final fields to prevent API breaking changes. #archcommitOutcome
No major behavior change is to be expected.
Potential Compatibility Issues
Technically the number of API change is one, but I'm pretty sure there's no person calling
Eases#valueOf()so I think this one is safe. If someone is callingEases#valueOf(), please contact me so I can change the number of people callingEases#valueOf()back to 0.Addons using
GTParticlesystem shouldcopeadjust the usage to new code. Same goes forBloomEffectUtil#scheduleBloomRender.