Conversation
fix: increment control
perf: cache console in generator
There was a problem hiding this comment.
Pull request overview
This pull request adds per-control animations for datapack consoles, allowing console controls to have customizable animations that reflect their state. The implementation introduces a new Transformations utility class, updates the control system to support animation references, and modifies rendering logic to apply per-control animations.
Changes:
- Added animation support to
ControlTypesallowing datapack consoles to define control-specific animations - Implemented
getTargetProgressmethod across all control classes to determine animation state - Created
Transformationsrecord to consolidate scale, offset, and rotation transformations
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tardim.geo.json | New Bedrock geometry file for a TARDIS model |
| Transformations.java | New utility class for managing model transformations (offset, scale, rotation) |
| ControlTypes.java | Added animation reference field and updated constructor to support per-control animations |
| Control.java | Added getTargetProgress method to determine animation progress based on control state |
| ControlRegistry.java | Added fallback method to return a default control when lookup fails |
| ConsoleControlEntity.java | Integrated animation state tracking and client/server synchronization for control animations |
| DatapackConsole.java | Updated to use Transformations class and support animation inheritance from parent consoles |
| ClientConsoleVariantRegistry.java | Enhanced texture/emission/model fallback logic with predicate-based parent lookup |
| ConsoleVariantRegistry.java | Updated syncing to use Transformations and added null safety check for parent() |
| BedrockConsoleModel.java | Implemented control animation application with caching and integration with console rendering |
| ConsoleBlockEntity.java | Added client-side control entity caching with periodic refresh for animation rendering |
| ConsoleRenderer.java | Changed render layer from EntityCutout to EntityTranslucentCull |
| ConsoleGeneratorRenderer.java | Added model caching and reordered transformation application |
| BedrockExteriorModel.java | Removed non-looping animation check that was logging errors |
| All Control implementations | Implemented getTargetProgress to expose control state for animations |
| IncrementControl.java, IncrementManager.java | Added methods to support animation progress calculation |
| PosControl.java | Changed shouldHaveDelay to return true |
| PowerControl.java | Removed unnecessary cast to ServerPlayerEntity |
| ControlAnimationState.java | Deleted file (replaced by TargetedAnimationState from amblekit) |
| gradle.properties | Updated amblekit version to support new animation features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import net.minecraft.entity.EntityDimensions; | ||
| import net.minecraft.util.Identifier; | ||
| import org.jetbrains.annotations.Nullable; | ||
| import org.joml.Vector3f; |
There was a problem hiding this comment.
Duplicate imports for EntityDimensions, Identifier, Vector3f, and Optional. These imports appear twice (lines 8/17, 10/18, 6/20, 22/14). Only one import for each is needed.
| import net.minecraft.entity.EntityDimensions; | |
| import net.minecraft.util.Identifier; | |
| import org.jetbrains.annotations.Nullable; | |
| import org.joml.Vector3f; | |
| import org.jetbrains.annotations.Nullable; |
| Control control = entity.getControl(); | ||
| if (control == null) return; | ||
|
|
||
| BedrockAnimation anim = this.animationCache.computeIfAbsent(entity.getControl().id(), k -> findAnimationById(entity)); |
There was a problem hiding this comment.
The animationCache uses the control ID as a key, but the lambda findAnimationById is only called once per control ID. If a control entity changes its animation reference, the cache will still return the old animation. Consider using a composite key that includes both the control ID and animation reference ID, or clearing the cache when animations change.
| if (cachedClientControls == null || this.age % CLIENT_CACHE_REFRESH_INTERVAL == 0) { | ||
| cachedClientControls = findControlEntitiesInWorld(); |
There was a problem hiding this comment.
The client-side cache refreshes every CLIENT_CACHE_REFRESH_INTERVAL ticks (2400 = 2 minutes). This means control entities that spawn or despawn won't be reflected in animations for up to 2 minutes. Consider reducing this interval or adding a mechanism to invalidate the cache when control entities are added/removed.
| "minecraft:geometry": [ | ||
| { | ||
| "description": { | ||
| "identifier": "geometry.unknown", |
There was a problem hiding this comment.
The geometry identifier is "geometry.unknown" (line 6). This should be changed to a more descriptive and unique identifier to avoid conflicts with other models and to make debugging easier. Consider using something like "geometry.tardim" or "geometry.ait.tardim".
| matrices.translate(-0.5, 1.5, 0.5); | ||
| bedrockConsoleModel.applyOffsets(matrices, entity.getConsoleVariant()); |
There was a problem hiding this comment.
The order of translate and applyOffsets was swapped. The translate is now before applyOffsets (line 92 before line 93), whereas in the old code it was after. This could change the visual positioning of the console model. Please verify this reordering is intentional and doesn't break the rendering.
| matrices.translate(-0.5, 1.5, 0.5); | |
| bedrockConsoleModel.applyOffsets(matrices, entity.getConsoleVariant()); | |
| bedrockConsoleModel.applyOffsets(matrices, entity.getConsoleVariant()); | |
| matrices.translate(-0.5, 1.5, 0.5); |
|
|
||
| @Environment(EnvType.CLIENT) | ||
| public void apply(MatrixStack stack) { | ||
| stack.translate(offset.x, offset.y * -1, offset.z); |
There was a problem hiding this comment.
The apply method performs offset.y * -1 (line 33), but this inversion is inconsistent with how offset.x and offset.z are handled. This might be intentional for coordinate system conversion, but should be documented in a comment to explain why Y is inverted.
|
loser review comments |
|
|
||
| private static final int CLIENT_CACHE_REFRESH_INTERVAL = 2400; // Lazy refresh | ||
| // Client-side cache for control entities | ||
| private List<ConsoleControlEntity> cachedClientControls = null; |
There was a problem hiding this comment.
do you even need this on the client?
There was a problem hiding this comment.
yes so the renderer can update the consoles controls
There was a problem hiding this comment.
arent control animations part of the console model
since the control entities arejust pretty much fancy hitboxes?
There was a problem hiding this comment.
it needs it because the control entities hold the control data
|
|
||
| private static final int CLIENT_CACHE_REFRESH_INTERVAL = 2400; // Lazy refresh | ||
| // Client-side cache for control entities | ||
| private List<ConsoleControlEntity> cachedClientControls = null; |
|
|
||
| private static final int CLIENT_CACHE_REFRESH_INTERVAL = 2400; // Lazy refresh | ||
| // Client-side cache for control entities | ||
| private List<ConsoleControlEntity> cachedClientControls = null; |
There was a problem hiding this comment.
arent control animations part of the console model
since the control entities arejust pretty much fancy hitboxes?
…oleModel.java Co-authored-by: Theo <git@theo.is-a.dev>
|
Old and also AI generated. Either finish or merge into the Redux assets PR. |
|
closes #1953