Various smaller improvements #350
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant CinematicSystem
participant WorldBorder
participant PacketSender
Player->>CinematicSystem: Start cinematic with WorldborderCinematicEntry
CinematicSystem->>WorldBorder: Determine segment (start/end frames, sizes)
CinematicSystem->>PacketSender: Send world border size/lerp packets
PacketSender->>Player: Animate border transition
CinematicSystem->>PacketSender: On segment end, reset or fade back border
PacketSender->>Player: Final border state update
sequenceDiagram
participant Player
participant FactEntry
participant CinematicSystem
Player->>FactEntry: Query CinematicSectionFactEntry
FactEntry->>CinematicSystem: Get current cinematic and frame
CinematicSystem-->>FactEntry: Return frame info
FactEntry->>Player: Return value for matching frame range or default
sequenceDiagram
participant WrapperEntity
participant ArmorStandEntity
participant InvisibleData
WrapperEntity->>ArmorStandEntity: applyProperty(property)
ArmorStandEntity->>InvisibleData: applyInvisibleData(entity, property)
InvisibleData->>WrapperEntity: Set invisibility meta
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/cinematic/WorldborderCinematicEntry.kt (1)
75-77: Consider improving type safety for polymorphic packet handling.The current code assigns different packet types to the same variable, which could lead to type confusion. Consider making the packet creation and sending more explicit.
- val packet = if (!segment.fadeBack) WrapperPlayServerWorldBorderSize(borderSize()) - else WrapperPlayWorldBorderLerpSize(segment.newSize, borderSize(), segment.transitionTime.toMillis()) - player.sendPacket(packet) + if (!segment.fadeBack) { + val packet = WrapperPlayServerWorldBorderSize(borderSize()) + player.sendPacket(packet) + } else { + val packet = WrapperPlayWorldBorderLerpSize(segment.newSize, borderSize(), segment.transitionTime.toMillis()) + player.sendPacket(packet) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/entry/temporal/TemporalInteraction.kt(2 hunks)extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/cinematic/WorldborderCinematicEntry.kt(1 hunks)extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/PlayerRespawnEventEntry.kt(2 hunks)extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/InCinematicFactEntry.kt(2 hunks)extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/data/minecraft/living/armorstand/InvisibleData.kt(1 hunks)extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/entity/minecraft/ArmorStandEntity.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/entity/minecraft/ArmorStandEntity.kt (1)
extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/data/minecraft/living/armorstand/InvisibleData.kt (1)
applyInvisibleData(33-38)
extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/data/minecraft/living/armorstand/InvisibleData.kt (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/dsl/TypewriterDsl.kt (1)
error(21-23)
🔇 Additional comments (12)
extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/data/minecraft/living/armorstand/InvisibleData.kt (3)
1-15: LGTM!The package declaration and imports are well-organized and appropriate for the armor stand invisibility functionality.
16-27: Well-structured entity data implementation.The
InvisibleDataclass follows the established patterns for entity data entries with proper annotations, type safety, and builder pattern implementation.
29-31: Clean property implementation.The
InvisiblePropertydata class and companion object follow the established patterns for entity properties with appropriate default values.extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/entity/minecraft/ArmorStandEntity.kt (1)
63-63: Proper integration pattern for the new property type.The addition of
InvisiblePropertyhandling follows the established pattern used by other armor stand properties. However, note that theapplyInvisibleDatafunction currently has an error handling issue that needs to be fixed.extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/PlayerRespawnEventEntry.kt (2)
42-42: LGTM: Clean enum extension for handling respawn without designated location.The addition of
NONEenum value appropriately addresses the use case where players don't have a bed or respawn anchor.
64-64: LGTM: Correct logic for NONE respawn location type.The condition
!event.isBedSpawn && !event.isAnchorSpawnproperly identifies respawn events that don't involve a bed or anchor, which aligns with the NONE semantics.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/entry/temporal/TemporalInteraction.kt (2)
38-38: LGTM: Appropriate visibility change to support frame querying.Making the
frameproperty public enables external components to access the current frame, which is required for the frame range filtering functionality inInCinematicFactEntry.
141-141: LGTM: Clean extension function for frame access.The
currentTemporalFrame()extension provides a convenient and null-safe way to access the current frame of a player's active temporal interaction.extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/InCinematicFactEntry.kt (1)
41-42: LGTM: Well-documented optional frame range parameters.The addition of
beforeFrameandafterFrameparameters with clear documentation enhances the cinematic fact filtering capabilities.extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/cinematic/WorldborderCinematicEntry.kt (3)
19-28: Excellent documentation and clear use case explanation.The entry documentation clearly explains the client-side nature of the animation and provides a practical use case example.
44-51: Well-designed segment structure with appropriate defaults.The
WorldborderSegmentdata class provides a comprehensive set of parameters with sensible defaults for world border animation control.
60-66: LGTM: Correct implementation of border size lerping.The
startSegmentmethod properly handles the transition from old size (or current border size if not specified) to the new size using the lerp packet.
...ain/kotlin/com/typewritermc/entity/entries/data/minecraft/living/armorstand/InvisibleData.kt
Show resolved
Hide resolved
...s/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/InCinematicFactEntry.kt
Outdated
Show resolved
Hide resolved
...s/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/InCinematicFactEntry.kt
Show resolved
Hide resolved
...ain/kotlin/com/typewritermc/entity/entries/data/minecraft/living/armorstand/InvisibleData.kt
Outdated
Show resolved
Hide resolved
...ension/src/main/kotlin/com/typewritermc/basic/entries/cinematic/WorldborderCinematicEntry.kt
Show resolved
Hide resolved
|
Thanks for this PR, it's great to see other people help out with Typewriter! Next time I would prefer you make 4 separate PRs for each feature because now we have multiple conversations next to each other. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/CinematicSectionFactEntry.kt (1)
52-58: Shadowing thedefaultproperty hurts readability.Inside
readSinglePlayera localval default = …hides the constructor property of the same name; readers have to check scopes twice.
Pick a distinct name (e.g.fallbackValue) or reuse the property directly.- val default = default.orElse(0) + val fallback = default.orElse(0) … - if (!inCinematic) return FactData(default) + if (!inCinematic) return FactData(fallback) … - return FactData(output ?: default) + return FactData(output ?: fallback)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/CinematicSectionFactEntry.kt(1 hunks)extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/InCinematicFactEntry.kt(1 hunks)extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/data/minecraft/living/armorstand/InvisibleData.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/InCinematicFactEntry.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/data/minecraft/living/armorstand/InvisibleData.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/CinematicSectionFactEntry.kt (1)
engine/engine-core/src/main/kotlin/com/typewritermc/core/entries/EntryReference.kt (1)
emptyRef(6-6)
...icExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/CinematicSectionFactEntry.kt
Show resolved
Hide resolved
...icExtension/src/main/kotlin/com/typewritermc/basic/entries/fact/CinematicSectionFactEntry.kt
Outdated
Show resolved
Hide resolved
2 participants
We've been using Typewriter on our (in-development) server for a while now and really love the plugin, but while using it, we've come across some minor issues or missing features that we needed. Most of those have been implemented in this PR, and we've been using them on the server for a few days now.
Here's a detailed list of the changes made:
PlayerRespawnEventEntrydoes not trigger at all if the player doesn't have a respawn point (such as a bed / respawn anchor), so I added aNONErespawn location type that covers this caseInvisibleNBT tag can't be applied to "fake" armor stand entities (which would hide the wood frame of the armor stand, but not the worn armor), this can now be achieved usingInvisibleDataWorldborderCinematicEntry. A segment of that cinematic will resize the worldborder client-side for a given amount of time, and return it to the previous size when the segment ends.beforeFrameandafterFrameparameters to the fact, which can be used to narrow down the frame range in a cinematic.Summary by CodeRabbit
New Features
Improvements