Feat: Adding some Item Components #381
Conversation
WalkthroughAdds many new ItemComponent implementations, two sealed interfaces for custom-model-data and PDC types, multiple concrete CustomModelData/PdcDataType classes, and a server-version–aware change to ItemNameComponent.matches. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player
participant Component as ItemComponent
participant VarResolver as Var
participant Item as ItemStack
participant Meta as ItemMeta
rect #EEFAFF
note over Player,Component: Apply flow (resolve & mutate)
Player->>Component: apply(player, ctx, item)
Component->>VarResolver: get(player, ctx)
VarResolver-->>Component: resolved value
Component->>Item: item.editMeta { mutate fields / PDC / custom model data }
Item->>Meta: read/modify metadata
Meta-->>Item: updated meta
end
rect #FFF8EE
note over Player,Component: Match flow (resolve & compare)
Player->>Component: matches(player, ctx, item)
Component->>VarResolver: get(player, ctx)
VarResolver-->>Component: expected value
Component->>Item: read item.itemMeta (PDC/customModelData/fields)
Item-->>Component: actual meta state
Component-->>Player: boolean (expected == actual)
end
sequenceDiagram
autonumber
participant NameComp as ItemNameComponent
participant Item as ItemStack
participant Server as ServerVersion
NameComp->>Server: isNewerThanOrEquals(V_1_21_4)?
alt newer-or-equal
NameComp->>Item: effectiveName().plainText()
else older
NameComp->>Item: displayName().plainText()
end
NameComp->>NameComp: parse placeholders and compare
NameComp-->>Caller: boolean (match)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)engine/{engine-core,engine-loader,engine-paper}/src/**/*.kt📄 CodeRabbit inference engine (engine/AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-08-24T11:11:05.217ZApplied to files:
🔇 Additional comments (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemJukeboxPlayableComponent.kt (1)
20-26: Avoid writing a no-op or invalid jukebox component when sound resolves to EMPTY.If
Sound.EMPTYrepresents “no song,” consider skipping the write (or clearing the component) instead of settingminecraft:empty. This prevents persisting a meaningless value and reduces churn.Apply this minimal no-op guard:
item.editMeta { meta -> - val soundKey = sound.get(player, interactionContext)?.soundId?.namespacedKey ?: return@editMeta + val resolvedSound = sound.get(player, interactionContext) ?: return@editMeta + if (resolvedSound == Sound.EMPTY) return@editMeta + val soundKey = resolvedSound.soundId?.namespacedKey ?: return@editMetaIf your intended behavior is to clear the component when empty, replace the early return with a proper clear according to the Paper API you target.
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt (2)
23-27: Use InteractionContext in Var resolution and avoid unnecessary writes when unchanged.For parity with other components and to support context-aware values, pass
interactionContext. Also, skip setting when the value is already applied.item.editMeta { meta -> - val namespaceValue = namespace.get(player) ?: return@editMeta - val keyValue = key.get(player) ?: return@editMeta + val namespaceValue = namespace.get(player, interactionContext) ?: return@editMeta + val keyValue = key.get(player, interactionContext) ?: return@editMeta val namespacedKey = NamespacedKey(namespaceValue, keyValue) + if (meta.tooltipStyle == namespacedKey) return@editMeta meta.tooltipStyle = namespacedKey }
31-37: Match should resolve using InteractionContext as well.Keeps
apply/matchessymmetry and enables context-dependent comparisons.- val expectedNamespace = namespace.get(player) ?: return false - val expectedKey = key.get(player) ?: return false + val expectedNamespace = namespace.get(player, interactionContext) ?: return false + val expectedKey = key.get(player, interactionContext) ?: return falseengine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt (2)
26-36: Use InteractionContext and treat blank values as “unset” for removal.
- Pass
interactionContextto enable context-aware values.- Using
isBlank()instead ofisEmpty()handles whitespace-only input gracefully.- val namespaceValue = namespace.get(player) ?: return@editMeta - val keyValue = key.get(player) ?: return@editMeta - val valueValue = value.get(player) ?: return@editMeta + val namespaceValue = namespace.get(player, interactionContext) ?: return@editMeta + val keyValue = key.get(player, interactionContext) ?: return@editMeta + val valueValue = value.get(player, interactionContext) ?: return@editMeta @@ - if (valueValue.isEmpty()) { + if (valueValue.isBlank()) { meta.persistentDataContainer.remove(namespacedKey) } else { meta.persistentDataContainer.set(namespacedKey, PersistentDataType.STRING, valueValue) }
41-53: Keepmatchessymmetric withapplyand align “blank” semantics.Mirror the context usage and treat blanks as “unset”.
- val expectedNamespace = namespace.get(player) ?: return false - val expectedKey = key.get(player) ?: return false - val expectedValue = value.get(player) ?: return false + val expectedNamespace = namespace.get(player, interactionContext) ?: return false + val expectedKey = key.get(player, interactionContext) ?: return false + val expectedValue = value.get(player, interactionContext) ?: return false @@ - return if (expectedValue.isEmpty()) { + return if (expectedValue.isBlank()) { actualValue == null } else { actualValue == expectedValue }engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt (2)
25-34: Reduce metadata churn: compute expected once, skip set when unchanged.This avoids unnecessary writes when the component is already in the desired state.
- val modelData = customModelData.get(player) ?: return@editMeta - val modelDataComponent = meta.customModelDataComponent - - if (modelData == 0) { - modelDataComponent.floats = emptyList() - } else { - modelDataComponent.floats = listOf(modelData.toFloat()) - } - meta.setCustomModelDataComponent(modelDataComponent) + val modelData = customModelData.get(player, interactionContext) ?: return@editMeta + val expectedFloats = if (modelData == 0) emptyList() else listOf(modelData.toFloat()) + val component = meta.customModelDataComponent + if (component.floats == expectedFloats) return@editMeta + component.floats = expectedFloats + meta.setCustomModelDataComponent(component)
38-49: Use InteractionContext for parity with apply.Keeps resolution symmetric and enables context-dependent matching.
- val expectedModelData = customModelData.get(player) ?: return false + val expectedModelData = customModelData.get(player, interactionContext) ?: return false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemJukeboxPlayableComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt(1 hunks)
🔇 Additional comments (4)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemJukeboxPlayableComponent.kt (2)
29-33: LGTM on equality check.Comparing
NamespacedKeydirectly is correct and keepsmatchesstrict.
22-25: Paper API version confirmed (1.21.x) – no changes requiredThe
engine-papermodule is already targeting Paper 1.21.x, which includes the data‐component API needed forjukeboxPlayable/songKey:
engine/engine-paper/build.gradle.kts
compileOnlyApi("io.papermc.paper:paper-api:1.21.8-R0.1-SNAPSHOT")(line 30)apiVersion = "1.21.3"(line 158)engine/gradle.properties
paperVersion=1.21.3-1.21.8(line 1)Since the build is on Paper 1.21.x+, the
jukeboxPlayablecomponent APIs are available and this will compile as-is.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt (1)
14-20: Defaults look consistent.
@Default("\"minecraft\"")and@Default("\"default\"")align with the providedConstVardefaults.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt (1)
28-46: Strict float-list equality is fine; confirm intended semantics.If other tooling might append additional floats, strict equality could produce false negatives. If the intent is “first float equals expected,” adjust accordingly; otherwise, current behavior is acceptable.
Would you like a variant of
matchesthat only checks the first element when non-zero?
...aper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt (1)
15-22: Resolved: default annotations now align with actual defaults.Thanks for fixing the namespace default;
@Default("\"typewriter\"")now matchesConstVar("typewriter"). The overall constructor surface looks consistent and editor-friendly.
🧹 Nitpick comments (4)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt (4)
21-22: Add a @default for value for consistency with other properties.To keep codegen/editors in sync and communicate the empty-string default, annotate
valueas well.- val value: Var<String> = ConstVar(""), + @Default("\"\"") + val value: Var<String> = ConstVar(""),
24-38: Harden key construction and input handling (trim + guard invalid NamespacedKey).End-user/Var inputs can include whitespace or illegal characters, and
NamespacedKey(ns, key)can throwIllegalArgumentException. Trim inputs and guard the constructor to avoid silent no-ops or exceptions insideeditMeta.- item.editMeta { meta -> - val namespaceValue = namespace.get(player, interactionContext) ?: return@editMeta - val keyValue = key.get(player, interactionContext) ?: return@editMeta - val valueValue = value.get(player, interactionContext) ?: return@editMeta - - val namespacedKey = NamespacedKey(namespaceValue, keyValue) - - if (valueValue.isBlank()) { - meta.persistentDataContainer.remove(namespacedKey) - } else { - meta.persistentDataContainer.set(namespacedKey, PersistentDataType.STRING, valueValue) - } - } + item.editMeta { meta -> + val ns = namespace.get(player, interactionContext)?.trim()?.takeIf { it.isNotEmpty() } ?: return@editMeta + val k = key.get(player, interactionContext)?.trim()?.takeIf { it.isNotEmpty() } ?: return@editMeta + val v = value.get(player, interactionContext)?.trim() ?: return@editMeta + + val namespacedKey = try { + NamespacedKey(ns, k) + } catch (_: IllegalArgumentException) { + return@editMeta + } + + if (v.isBlank()) { + meta.persistentDataContainer.remove(namespacedKey) + } else { + meta.persistentDataContainer.set(namespacedKey, PersistentDataType.STRING, v) + } + }
40-53: Mirror the same robustness in matches (trim + safe key construction + tolerant blank semantics).
- Trim resolved values and treat empty-after-trim as absent.
- Guard
NamespacedKeycreation; returnfalseon invalid inputs.- If the expected value is blank, consider a stored blank as a match too, not only
null(external code might have set""instead of removing the key).- val expectedNamespace = namespace.get(player, interactionContext) ?: return false - val expectedKey = key.get(player, interactionContext) ?: return false - val expectedValue = value.get(player, interactionContext) ?: return false - - val namespacedKey = NamespacedKey(expectedNamespace, expectedKey) - val actualValue = item.itemMeta?.persistentDataContainer?.get(namespacedKey, PersistentDataType.STRING) - - return if (expectedValue.isBlank()) { - actualValue == null - } else { - actualValue == expectedValue - } + val ns = namespace.get(player, interactionContext)?.trim()?.takeIf { it.isNotEmpty() } ?: return false + val k = key.get(player, interactionContext)?.trim()?.takeIf { it.isNotEmpty() } ?: return false + val expected = value.get(player, interactionContext)?.trim() ?: return false + + val namespacedKey = try { + NamespacedKey(ns, k) + } catch (_: IllegalArgumentException) { + return false + } + val actualValue = item.itemMeta?.persistentDataContainer?.get(namespacedKey, PersistentDataType.STRING) + + return if (expected.isBlank()) { + actualValue.isNullOrBlank() + } else { + actualValue == expected + }
24-53: Add focused tests for apply/matches symmetry and edge cases.Minimal cases to cover:
- Writes string value and is matched.
- Blank expected -> removes key and matches when missing; also matches if an external plugin left
"".- Trimming inputs:
" key "/" value "behave as expected.- Invalid key/namespace characters do not crash
applyand result inmatches == false.I can scaffold parameterized tests using a fake
Var<String>and a testItemStackfactory if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemJukeboxPlayableComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemJukeboxPlayableComponent.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt
🔇 Additional comments (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt (1)
30-31: Verified: NamespacedKey(String, String) is supported in your target Paper APIYour module’s build file declares
compileOnlyApi("io.papermc.paper:paper-api:1.21.8-R0.1-SNAPSHOT")(engine/engine-paper/build.gradle.kts:30), and the Spigot/Paper Javadoc for that exact version lists both
NamespacedKey(String namespace, String key)
and
NamespacedKey(Plugin plugin, String key)
constructors inorg.bukkit.NamespacedKey(hub.spigotmc.org). No changes needed.
…ple white spaces or illegal chars)
Chaosgh
changed the title
...n/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt
Outdated
Show resolved
Hide resolved
...n/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt
Outdated
Show resolved
Hide resolved
...aper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt
Outdated
Show resolved
Hide resolved
...main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt
Outdated
Show resolved
Hide resolved
|
Is it possible to add the ItemModel component? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt (4)
17-21: Input type should be future-proof (floats list) per maintainer’s noteOwner requested floats list input to avoid locking into ints. Consider
List<Var<Float>>now.- @InnerMin(Min(0)) - @Default("0") - val customModelData: Var<Int> = ConstVar(0), + // Each entry represents a layer; empty list means "clear" + val customModelData: List<Var<Float>> = emptyList(),If you want, I can push a follow-up patch that maps this list to:
- new API:
component.floats = resolvedList- legacy API: first element’s floor/int (or error if >1 provided)
2-2: Remove stray blank line to match styleMinor style nit; previous review mentioned this too.
-
22-31: Backward-compat bug: breaks on 1.21.3 (uses only the new ItemComponent API)Current
applypath unconditionally touchescustomModelDataComponent, which likely doesn’t exist on 1.21.3. Gate by server version and fall back to legacysetCustomModelData(Int?).+import com.github.retrooper.packetevents.manager.server.ServerVersion +import com.typewritermc.engine.paper.utils.serverVersion @@ override fun apply(player: Player?, interactionContext: InteractionContext?, item: ItemStack) { item.editMeta { meta -> val modelData = customModelData.get(player, interactionContext) ?: return@editMeta - val expectedFloats = if (modelData == 0) emptyList() else listOf(modelData.toFloat()) - val component = meta.customModelDataComponent - if (component.floats == expectedFloats) return@editMeta - component.floats = expectedFloats - meta.setCustomModelDataComponent(component) + if (serverVersion.isNewerThan(ServerVersion.V_1_21_3)) { + val expectedFloats = if (modelData == 0) emptyList() else listOf(modelData.toFloat()) + val component = meta.customModelDataComponent + if (component.floats == expectedFloats) return@editMeta + component.floats = expectedFloats + meta.setCustomModelDataComponent(component) + } else { + val expectedInt: Int? = modelData.takeUnless { it == 0 } + val currentInt: Int? = if (meta.hasCustomModelData()) meta.customModelData else null + if (currentInt == expectedInt) return@editMeta + meta.setCustomModelData(expectedInt) + } } }
32-44: Backward-compat bug in matches: only checks new component pathMirror the apply-branching in
matches()to support 1.21.3 with legacy API.override fun matches(player: Player?, interactionContext: InteractionContext?, item: ItemStack): Boolean { val expectedModelData = customModelData.get(player, interactionContext) ?: return false - val actualComponent = item.itemMeta?.customModelDataComponent - val actualFloats = actualComponent?.floats ?: emptyList() - - val expectedFloats = if (expectedModelData == 0) { - emptyList() - } else { - listOf(expectedModelData.toFloat()) - } - - return actualFloats == expectedFloats + return if (serverVersion.isNewerThan(ServerVersion.V_1_21_3)) { + val actualComponent = item.itemMeta?.customModelDataComponent + val actualFloats = actualComponent?.floats ?: emptyList() + val expectedFloats = if (expectedModelData == 0) emptyList() else listOf(expectedModelData.toFloat()) + actualFloats == expectedFloats + } else { + val meta = item.itemMeta ?: return false + val actualInt: Int? = if (meta.hasCustomModelData()) meta.customModelData else null + val expectedInt: Int? = expectedModelData.takeUnless { it == 0 } + actualInt == expectedInt + } }
🧹 Nitpick comments (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemNameComponent.kt (1)
37-41: Guard version boundary and avoid duplicate placeholder parsing
- Confirm the exact API cutoff where
effectiveName()became available. If it exists in 1.21.3, switch to an inclusive check; otherwise current check is fine. Also, compute the expected name once to avoid parsing twice.- return if (serverVersion.isNewerThan(ServerVersion.V_1_21_3)) { - item.effectiveName().plainText() == name.parsePlaceholders(player).stripped() - } else { - item.displayName().plainText() == name.parsePlaceholders(player).stripped() - } + val expected = name.parsePlaceholders(player).stripped() + return if (serverVersion.isNewerThan(ServerVersion.V_1_21_3)) { + item.effectiveName().plainText() == expected + } else { + item.displayName().plainText() == expected + }Follow-up:
- If
displayName()can be null on older APIs, consider a null-safe comparison to avoid NPEs. Do you want me to patch that defensively once the boundary is confirmed?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemNameComponent.kt(3 hunks)
🔇 Additional comments (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemNameComponent.kt (1)
3-3: LGTM: version-aware importsImports for
ServerVersionandserverVersionare appropriate for the new conditional path.Also applies to: 15-15
Shouldn't be too hard, i will need to look up if its in 1.21.3 |
- Changed all namespace keys to a single string for user-friendliness. - Added an item model component.
- Changed all namespace keys to a single string for user-friendliness. - Added an item model component.
Components
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt (1)
13-50: Gate by server support to avoid NoSuchMethodError on pre‑1.21.5.Per prior feedback, guard both apply/matches before touching tooltipStyle and fail fast with a clear message.
Apply:
@@ import org.bukkit.inventory.ItemStack +// Runtime capability gate (prevents NoSuchMethodError on older servers) +private val SUPPORTS_TOOLTIP_STYLE by lazy { + try { + Class.forName("org.bukkit.inventory.meta.ItemMeta").getMethod("getTooltipStyle") + true + } catch (_: Throwable) { + false + } +} + @AlgebraicTypeInfo("tooltip_style", Colors.CYAN, "fa6-solid:palette") class ItemTooltipStyleComponent( @@ override fun apply(player: Player?, interactionContext: InteractionContext?, item: ItemStack) { + if (!SUPPORTS_TOOLTIP_STYLE) { + error("tooltip_style is unsupported on your server version. Requires Paper 1.21.5+.") + } item.editMeta { meta -> @@ override fun matches(player: Player?, interactionContext: InteractionContext?, item: ItemStack): Boolean { + if (!SUPPORTS_TOOLTIP_STYLE) { + error("tooltip_style is unsupported on your server version. Requires Paper 1.21.5+.") + } val raw = styleKey.get(player, interactionContext)?.trim().orEmpty()engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt (1)
2-2: Remove stray empty line after package.Matches prior feedback; keeps file headers consistent.
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt (1)
15-20: Scope agreement: ensure coverage of all non-recursive PDC primitives.Echoing prior feedback: this component is most useful if every non-recursive PersistentDataType is supported (BYTE, SHORT, INTEGER, LONG, FLOAT, DOUBLE, STRING, BYTE_ARRAY, INTEGER_ARRAY, LONG_ARRAY). Otherwise, consider deferring the component.
Run to check current coverage:
#!/bin/bash set -euo pipefail echo "Implemented PdcDataType classes:" rg -nP --type=kotlin 'class\s+(?:data\s+)?([A-Za-z0-9_]+PdcData)\b' | sed -nE 's/.*class (?:data )?([A-Za-z0-9_]+PdcData).*/\1/p' | sort -u echo required=(BytePdcData ShortPdcData IntPdcData LongPdcData FloatPdcData DoublePdcData StringPdcData ByteArrayPdcData IntArrayPdcData LongArrayPdcData) for t in "${required[@]}"; do if rg -nP --type=kotlin "\bclass\s+(?:data\s+)?$t\b" >/dev/null; then echo "OK - $t" else echo "MISS- $t" fi done
🧹 Nitpick comments (22)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/LegacyCustomModelData.kt (1)
15-22: Avoid unnecessary meta writes; simplify matches.Minor optimization and cleaner equality.
Apply:
@@ - override fun apply(player: Player?, interactionContext: InteractionContext?, item: ItemStack) { - item.editMeta { meta -> meta.setCustomModelData(value) } - } + override fun apply(player: Player?, interactionContext: InteractionContext?, item: ItemStack) { + item.editMeta { meta -> + if (meta.customModelData != value) { + meta.setCustomModelData(value) + } + } + } @@ - val meta = item.itemMeta ?: return false - return meta.hasCustomModelData() && meta.customModelData == value + val meta = item.itemMeta ?: return false + return meta.customModelData == valueengine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/FloatPdcData.kt (2)
29-31: Handle Float edge cases (NaN) and consider tolerance in matches.Direct
==fails forNaN. Optionally also allow a tiny epsilon to avoid false negatives from rounding.- return container.get(key, PersistentDataType.FLOAT) == value + val existing = container.get(key, PersistentDataType.FLOAT) ?: return false + if (existing.isNaN() && value.isNaN()) return true + return existing == value + // Optional tolerance: + // return abs(existing - value) <= 1e-6f
17-21: Skip write when unchanged to reduce meta churn.Small optimization to avoid needless
editMetawrites.- item.editMeta { meta -> - meta.persistentDataContainer.set(key, PersistentDataType.FLOAT, value) - } + item.editMeta { meta -> + val pdc = meta.persistentDataContainer + if (pdc.get(key, PersistentDataType.FLOAT) == value) return@editMeta + pdc.set(key, PersistentDataType.FLOAT, value) + }engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/BooleansCustomModelData.kt (1)
30-37: Avoid logging warnings inside matches; it can spam logs under frequent checks.Return false silently in
matchesand keep the warning inapplyonly.- if (!serverVersion.isNewerThan(ServerVersion.V_1_21_3)) { - logger.warning("${this::class.simpleName} is only supported in versions higher than 1.21.3") - return false - } + if (!serverVersion.isNewerThan(ServerVersion.V_1_21_3)) { + return false + }engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BytePdcData.kt (2)
36-45: Remove redundant equals/hashCode; data class already provides them.Less code, same behavior.
- override fun equals(other: Any?): Boolean { - if (this === other) return true - if (javaClass != other?.javaClass) return false - - other as BytePdcData - return value == other.value - } - - override fun hashCode(): Int = value
18-23: Optional: store as Byte at the API to match the persisted type.If caller-side ergonomics allow, using
value: Byteremoves conversion and range concerns.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntPdcData.kt (1)
17-21: Skip write when unchanged.Minor save on allocations and NBT churn.
- item.editMeta { meta -> - meta.persistentDataContainer.set(key, PersistentDataType.INTEGER, value) - } + item.editMeta { meta -> + val pdc = meta.persistentDataContainer + if (pdc.get(key, PersistentDataType.INTEGER) == value) return@editMeta + pdc.set(key, PersistentDataType.INTEGER, value) + }engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/StringPdcData.kt (1)
19-24: Avoid treating whitespace-only strings as “unset” unless intentionalUsing isBlank() removes values like " ". If that’s not desired, prefer isEmpty() to only treat truly empty strings as “unset,” and mirror it in matches().
- if (value.isBlank()) { + if (value.isEmpty()) { meta.persistentDataContainer.remove(key) } else { meta.persistentDataContainer.set(key, PersistentDataType.STRING, value) } ... - return if (value.isBlank()) { - actual.isNullOrBlank() + return if (value.isEmpty()) { + actual.isNullOrEmpty() } else { actual == value }Also applies to: 34-38
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/DoublePdcData.kt (1)
23-31: Use tolerance when comparing doublesExact equality on doubles is brittle. Suggest epsilon-based comparison.
- val container = item.itemMeta?.persistentDataContainer ?: return false - return container.get(key, PersistentDataType.DOUBLE) == value + val container = item.itemMeta?.persistentDataContainer ?: return false + val actual = container.get(key, PersistentDataType.DOUBLE) ?: return false + return kotlin.math.abs(actual - value) <= 1e-9Also, consider aligning “absence semantics” with StringPdcData (blank→absent). Do we want “unset” to ever match a default (e.g., 0.0)? If yes, handle null accordingly.
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/ColorsCustomModelData.kt (2)
20-23: Version gate boundary and log noise
Confirm the threshold: is the feature “≥ 1.21.4” or “> 1.21.3”? The code enforces “> 1.21.3”. Validate against the API you target.
Repeated warnings on every call can spam logs. Consider throttling (one-time warning) or lowering to fine/debug.
Also applies to: 35-38
26-27: Deduplicate color mappingThe toBukkitColor mapping is duplicated. Extract a private helper to keep apply/matches consistent and cheaper to change.
Also applies to: 40-41
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BooleanPdcData.kt (1)
17-21: Decide on “absence” semantics for booleansCurrently, apply always sets the key and matches requires presence. StringPdcData treats “blank” as key removal and matches null/blank. For consistency and smaller NBT, consider removing the key when value == false and letting matches treat null as false.
- item.editMeta { meta -> - meta.persistentDataContainer.set(key, PersistentDataType.BOOLEAN, value) - } + item.editMeta { meta -> + val pdc = meta.persistentDataContainer + if (!value) pdc.remove(key) else pdc.set(key, PersistentDataType.BOOLEAN, value) + } ... - val container = item.itemMeta?.persistentDataContainer ?: return false - return container.get(key, PersistentDataType.BOOLEAN) == value + val container = item.itemMeta?.persistentDataContainer ?: return !value + val actual = container.get(key, PersistentDataType.BOOLEAN) + return (actual ?: false) == valueAlso confirm PersistentDataType.BOOLEAN is available for your minimum supported server version.
Also applies to: 29-31
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/FloatsCustomModelData.kt (2)
31-38: Approximate comparison for float arraysList equality on Float is strict and may fail due to precision. Compare element-wise with epsilon.
- val meta = item.itemMeta ?: return false - return meta.customModelDataComponent.floats == value + val meta = item.itemMeta ?: return false + val actual = meta.customModelDataComponent.floats + if (actual.size != value.size) return false + return actual.zip(value).all { (a, b) -> kotlin.math.abs(a - b) <= 1e-6f }
32-35: Reduce warning spam on unsupported versionsSame note as ColorsCustomModelData: throttle or downgrade log level to avoid noisy logs when matches() is called frequently.
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/StringsCustomModelData.kt (1)
31-35: Reduce warn-level spam on unsupported servers.matches() may be called frequently; logging a warning every time can flood logs. Consider downgrading to debug or logging once.
Example:
- logger.warning("${this::class.simpleName} is only supported in versions higher than 1.21.3") + // Consider logger.fine/debug, or a one-time warning elsewhere.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ByteArrayPdcData.kt (4)
16-18: Clarify unsigned byte intent and enforce lower bound (0..255).Current annotation only caps at 255; negatives will silently wrap via
toByte(). If the intent is unsigned bytes, either add a min bound (0) or validate/mask during conversion.Apply one of:
- @InnerMax(Max(255)) - val value: List<Int> = emptyList() + @InnerMax(Max(255)) + val value: List<Int> = emptyList() // intended range: 0..255?And/or strengthen
toByteArray()below (see next comment). If you prefer annotations, I can wire in@InnerMin(Min(0))if available in this module.
21-23: Prevent silent overflow in Int→Byte mapping.Mask and optionally validate each element to keep semantics explicit for unsigned bytes.
- private fun toByteArray(): ByteArray = - value.map { it.toByte() }.toByteArray() + private fun toByteArray(): ByteArray = + ByteArray(value.size) { i -> + val v = value[i] + require(v in 0..255) { "byte_array expects values in 0..255, got $v at index $i" } + (v and 0xFF).toByte() + }
36-39: Minor: avoid recomputing expected array.Store
toByteArray()result once.- val actual = container.get(key, PersistentDataType.BYTE_ARRAY) ?: return false - return actual.contentEquals(toByteArray()) + val actual = container.get(key, PersistentDataType.BYTE_ARRAY) ?: return false + val expected = toByteArray() + return actual.contentEquals(expected)
41-49: Redundant equals/hashCode in a data class.Data classes already generate value-based
equals/hashCodeforvalue: List<Int>. These overrides can be dropped.- override fun equals(other: Any?): Boolean { - if (this === other) return true - if (javaClass != other?.javaClass) return false - - other as ByteArrayPdcData - return value == other.value - } - - override fun hashCode(): Int = value.hashCode() + // rely on data class-generated equals/hashCodeengine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt (1)
26-28: Support bare keys (no namespace) with a sane default.Right now, keys without a namespace are ignored. Consider defaulting to
typewriter:(or plugin namespace) for UX.- val namespacedKey = NamespacedKey.fromString(raw) ?: return + val namespacedKey = NamespacedKey.fromString(raw) + ?: if (':' !in raw) NamespacedKey.fromString("typewriter:$raw") else null + ?: returnIf you prefer plugin-based fallback (Paper ≥1.20): use
NamespacedKey.fromString(raw, plugin)where available.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntArrayPdcData.kt (2)
32-35: Minor: avoid recomputing expected array.- val actual = container.get(key, PersistentDataType.INTEGER_ARRAY) ?: return false - return actual.contentEquals(toIntArray()) + val actual = container.get(key, PersistentDataType.INTEGER_ARRAY) ?: return false + val expected = toIntArray() + return actual.contentEquals(expected)
37-45: Redundant equals/hashCode in a data class.These can be removed; data class-generated implementations suffice.
- override fun equals(other: Any?): Boolean { - if (this === other) return true - if (javaClass != other?.javaClass) return false - - other as IntArrayPdcData - return value == other.value - } - - override fun hashCode(): Int = value.hashCode() + // rely on data class-generated equals/hashCode
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemAmountComponent.kt(2 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemModelComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/BooleansCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/ColorsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/CustomModelDataType.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/FloatsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/LegacyCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/StringsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BooleanPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ByteArrayPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BytePdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/DoublePdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/FloatPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntArrayPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongArrayPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/PdcDataType.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ShortPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/StringPdcData.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-24T11:10:07.264Z
Learnt from: CR
PR: gabber235/Typewriter#0
File: .github/instructions/entity-extension.add-entity.instructions.md:0-0
Timestamp: 2025-08-24T11:10:07.264Z
Learning: Applies to extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/entity/minecraft/*Entity.kt : Ensure imports/types compile: Ref, Var, ConstVar, OnlyTags, and correct colors/icons/description in Entry
Applied to files:
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemAmountComponent.kt
📚 Learning: 2025-08-24T11:11:05.217Z
Learnt from: CR
PR: gabber235/Typewriter#0
File: .github/instructions/entity-extension.instructions.md:0-0
Timestamp: 2025-08-24T11:11:05.217Z
Learning: Applies to extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/**/*.kt : Use icons from established packs (lucide:, ph:, fa6-solid:, etc.) and colors from com.typewritermc.core.books.pages.Colors
Applied to files:
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemAmountComponent.kt
📚 Learning: 2025-08-24T11:11:05.217Z
Learnt from: CR
PR: gabber235/Typewriter#0
File: .github/instructions/entity-extension.instructions.md:0-0
Timestamp: 2025-08-24T11:11:05.217Z
Learning: Applies to extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/**/*.kt : Use Help and Default annotations to improve editor UX for ranges, booleans, durations, etc.
Applied to files:
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemAmountComponent.kt
📚 Learning: 2025-08-24T11:11:05.217Z
Learnt from: CR
PR: gabber235/Typewriter#0
File: .github/instructions/entity-extension.instructions.md:0-0
Timestamp: 2025-08-24T11:11:05.217Z
Learning: Applies to extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/**/*.kt : Annotate all entry classes with Entry(id, description, Colors, icon); use Tags on definition entries; use OnlyTags on data lists to constrain allowed data types
Applied to files:
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemAmountComponent.kt
🔇 Additional comments (10)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/CustomModelDataType.kt (1)
7-10: LGTM: clear, minimal contract for pluggable C(MD) strategies.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemAmountComponent.kt (1)
17-28: Enforce consistent default of 1 and clamp item amounts to [1, maxStackSize]
- Change
@InnerMin(Min(0))→@InnerMin(Min(1)).- In
applyandmatches, default to 1 (instead of 0) and clamp viacoerceIn(1, item.type.maxStackSize)to prevent invalid or broken ItemStacks (setting 0 makes the stack invisible/null [turn4search0] and falls outside Bukkit’s allowed range [turn3search0]).- Verify no existing configurations rely on
amount = 0to “clear” items, since 0 isn’t supported.@@ - @InnerMin(Min(0)) + @InnerMin(Min(1)) @@ - item.amount = amount.get(player, interactionContext) ?: 1 + val v = (amount.get(player, interactionContext) ?: 1) + .coerceIn(1, item.type.maxStackSize) + item.amount = v @@ - val amount = amount.get(player, interactionContext) ?: 0 - return item.amount == amount + val expected = (amount.get(player, interactionContext) ?: 1) + .coerceIn(1, item.type.maxStackSize) + return item.amount == expectedengine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/BooleansCustomModelData.kt (1)
18-21: Verify ServerVersion boundary semantics
Confirm whetherserverVersion.isNewerThan(ServerVersion.V_1_21_3)is strictly “> 1.21.3” or “≥ 1.21.3.” If it’s strict, update the warning to “1.21.4+”; otherwise switch to an at-least check (≥ 1.21.3) and adjust the message. Apply the same change across all CustomModelDataType classes for consistency.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/PdcDataType.kt (1)
8-11: Solid sealed interface design.Clear contract; simple and extensible.
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemCustomModelDataComponent.kt (2)
18-25: Delegation design looks good.Cleanly delegates to the selected CustomModelDataType and short-circuits on null. No issues spotted.
15-17: Verify serverVersion gating in all CustomModelDataType implementations. Manually confirm that everyCustomModelDataTypetouchingItemMetaAPIs added in 1.21.3+ includes its ownserverVersioncheck (asStringsCustomModelDatadoes) to avoid a potentialNoSuchMethodErroron 1.21.3.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/StringsCustomModelData.kt (1)
23-28: LGTM on meta mutation and equality check.Efficient early-return when already in desired state; equality on List uses structural equals.
Also applies to: 36-38
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongArrayPdcData.kt (2)
40-49: Equality/hashCode usage is appropriate for list-based value.No changes needed here.
14-18: Switch to Long-backed List and update bounds annotations
- Change
val value: List<Int>toList<Long>so you can represent full 64-bit values.- Replace
@InnerMin(Min(Long.MIN_VALUE.toInt()))/@InnerMax(Max(Long.MAX_VALUE.toInt()))with Long-based bounds (e.g.@InnerMin(Min(Long.MIN_VALUE))and@InnerMax(Max(Long.MAX_VALUE))).- If those annotations don’t yet accept Long parameters, add a runtime check in
init { require(value.all { it in Long.MIN_VALUE..Long.MAX_VALUE }) }.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntArrayPdcData.kt (1)
11-16: LGTM: matches the established PDC pattern.Type id, defaults, and constructor align with other PDC data types.
...pewritermc/engine/paper/utils/item/components/customModelDataTypes/StringsCustomModelData.kt
Outdated
Show resolved
Hide resolved
...er/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemModelComponent.kt
Show resolved
Hide resolved
.../src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BytePdcData.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt
Outdated
Show resolved
Hide resolved
...src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ShortPdcData.kt
Outdated
Show resolved
Hide resolved
- now just uses pure shorts and longs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ShortPdcData.kt (1)
41-49: Remove redundant equals/hashCode — data class already generates them.These overrides duplicate data-class defaults and add maintenance overhead. Rely on the generated implementations.
Apply:
- override fun equals(other: Any?): Boolean { - if (this === other) return true - if (javaClass != other?.javaClass) return false - - other as ShortPdcData - return value == other.value - } - - override fun hashCode(): Int = value.hashCode()engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt (1)
41-49: Redundant equals/hashCode in a data class — remove.Data classes already generate correct equals/hashCode for primary constructor properties.
Apply:
- override fun equals(other: Any?): Boolean { - if (this === other) return true - if (javaClass != other?.javaClass) return false - - other as LongPdcData - return value == other.value - } - - override fun hashCode(): Int = value.hashCode() + // Rely on data class defaults for equals/hashCode.
🧹 Nitpick comments (10)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ShortPdcData.kt (1)
14-16: Trim extra blank lines for compact declaration.Minor formatting nit to keep the primary constructor concise.
-data class ShortPdcData( - - - val value: Short = 0 +data class ShortPdcData( + val value: Short = 0engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongArrayPdcData.kt (3)
21-30: Optionally remove key when value is empty to keep PDC tidyWriting an empty array is valid, but removing the key reduces NBT noise and can help matching semantics (see next comment).
Apply:
- ) { - item.editMeta { meta -> - meta.persistentDataContainer.set(key, PersistentDataType.LONG_ARRAY, toLongArray()) - } - } + ) { + item.editMeta { meta -> + val container = meta.persistentDataContainer + if (value.isEmpty()) { + container.remove(key) + } else { + container.set(key, PersistentDataType.LONG_ARRAY, toLongArray()) + } + } + }
32-41: Match empty expected to “key absent” and avoid an extra allocationIf you choose to remove empty values, matching should treat “no key” as equivalent to an empty expected. Also cache the expected array to avoid converting twice.
Apply:
- ): Boolean { - val container = item.itemMeta?.persistentDataContainer ?: return false - val actual = container.get(key, PersistentDataType.LONG_ARRAY) ?: return false - return actual.contentEquals(toLongArray()) - } + ): Boolean { + val expected = toLongArray() + val container = item.itemMeta?.persistentDataContainer + ?: return expected.isEmpty() + val actual = container.get(key, PersistentDataType.LONG_ARRAY) + return when { + actual == null -> expected.isEmpty() + else -> actual.contentEquals(expected) + } + }
43-51: Drop manual equals/hashCode; data class already provides theseRedundant overrides add maintenance risk if fields change later.
Apply:
- override fun equals(other: Any?): Boolean { - if (this === other) return true - if (javaClass != other?.javaClass) return false - - other as LongArrayPdcData - return value == other.value - } - - override fun hashCode(): Int = value.hashCode()engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt (2)
20-29: Micro-opt: skip write if value is unchanged.Avoids unnecessary meta churn when the stored value already matches.
Apply:
- override fun apply( + @Suppress("UNUSED_PARAMETER") + override fun apply( player: Player?, interactionContext: InteractionContext?, item: ItemStack, key: NamespacedKey ) { item.editMeta { meta -> - meta.persistentDataContainer.set(key, PersistentDataType.LONG, value) + val pdc = meta.persistentDataContainer + if (pdc.get(key, PersistentDataType.LONG) != value) { + pdc.set(key, PersistentDataType.LONG, value) + } } }
20-25: Silence unused-parameter warnings (optional).These params are required by the interface but unused. Either keep the @Suppress shown above, or prefix names with underscores for consistency across implementations.
Also applies to: 31-36
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/BooleansCustomModelData.kt (1)
17-17: Silence unused parameter warnings on overrides.Interface forces these params; annotate to keep the file clean.
- override fun apply(player: Player?, interactionContext: InteractionContext?, item: ItemStack) { + @Suppress("UNUSED_PARAMETER") + override fun apply(player: Player?, interactionContext: InteractionContext?, item: ItemStack) { @@ - override fun matches(player: Player?, interactionContext: InteractionContext?, item: ItemStack): Boolean { + @Suppress("UNUSED_PARAMETER") + override fun matches(player: Player?, interactionContext: InteractionContext?, item: ItemStack): Boolean {Also applies to: 30-30
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/ColorsCustomModelData.kt (3)
20-23: Throttle or downgrade unsupported-version warningsBoth apply/matches warn on every call when < 1.21.4. This can spam logs. Consider logging once (per class) or downgrading to debug/fine.
Apply this minimal “log once” change:
data class ColorsCustomModelData( val value: List<Color> = emptyList() ) : CustomModelDataType { + companion object { + @Volatile private var warnedUnsupported = false + } override fun apply(player: Player?, interactionContext: InteractionContext?, item: ItemStack) { if (!serverVersion.isNewerThanOrEquals(ServerVersion.V_1_21_4)) { - logger.warning("${this::class.simpleName} is only supported in versions 1.21.4 and above") + if (!warnedUnsupported) { + warnedUnsupported = true + logger.warning("${this::class.simpleName} is only supported in versions 1.21.4 and above") + } return } ... override fun matches(player: Player?, interactionContext: InteractionContext?, item: ItemStack): Boolean { if (!serverVersion.isNewerThanOrEquals(ServerVersion.V_1_21_4)) { - logger.warning("${this::class.simpleName} is only supported in versions 1.21.4 and above") + if (!warnedUnsupported) { + warnedUnsupported = true + logger.warning("${this::class.simpleName} is only supported in versions 1.21.4 and above") + } return false }Also applies to: 35-38
26-27: DRY the color mappingThe value → Bukkit color mapping is duplicated. Extract a tiny helper to keep this consistent.
Add helper (top-level in this file):
private fun List<Color>.toBukkitColors() = map(Color::toBukkitColor)Then:
- val bukkitColors = value.map { it.toBukkitColor() } + val bukkitColors = value.toBukkitColors() ... - val bukkitColors = value.map { it.toBukkitColor() } + val bukkitColors = value.toBukkitColors()Also applies to: 40-41
19-19: Mark unused parameters to silence warningsThese are unused in this implementation; rename with underscores.
- override fun apply(player: Player?, interactionContext: InteractionContext?, item: ItemStack) { + override fun apply(_player: Player?, _interactionContext: InteractionContext?, item: ItemStack) { ... - override fun matches(player: Player?, interactionContext: InteractionContext?, item: ItemStack): Boolean { + override fun matches(_player: Player?, _interactionContext: InteractionContext?, item: ItemStack): Boolean {Also applies to: 34-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/BooleansCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/ColorsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/FloatsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/StringsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ByteArrayPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BytePdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongArrayPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ShortPdcData.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ByteArrayPdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BytePdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/FloatsCustomModelData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/StringsCustomModelData.kt
🔇 Additional comments (11)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ShortPdcData.kt (1)
16-16: Good: public API uses Short directly (no narrowing risks).Switching to
val value: Shortremoves overflow/wrap-around risks fromInt.toShort()paths and matchesPersistentDataType.SHORTcorrectly.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongArrayPdcData.kt (1)
11-16: LGTM: clean PDC type with correct LONG_ARRAY usageType metadata + data class shape look good and consistent with the other PDC types.
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt (2)
12-18: Good fix: use Long end-to-end for LONG PDC.This resolves the previous truncation risk and aligns storage with Bukkit’s PersistentDataType.LONG.
31-39: Confirm matching semantics for “absent vs. present=0L”.Currently returns false when the key is missing even if value == 0L. If “zero means unset” is desired elsewhere, consider treating missing as match for 0L to keep behavior consistent across PDC types. Otherwise, this is fine.
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/BooleansCustomModelData.kt (4)
22-27: LGTM: Efficient no-op short-circuit before write.The equality guard avoids unnecessary meta writes. Good.
12-16: AlgebraicTypeInfo id ‘boolean_array’ is unique and consistent
The id appears exactly once and matches the ‘_array’ naming pattern used by sibling types (e.g. int_array, float_array). No duplicates detected.
16-21: Extract and centralize version gating
- Define a shared
MIN_SUPPORTEDconstant (e.g.ServerVersion.V_1_21_3) in a companion object and replace all hard-codedV_1_21_4checks (9 occurrences across customModelDataTypes, seerg -nP 'isNewerThanOrEquals\(ServerVersion\.V_1_21_\d+\)').- Add a
warnOncehelper in the companion to emit the warning only once per runtime.- Verify the actual minimal Paper/MC version where
CustomModelDataComponent.flagswas introduced before finalizingMIN_SUPPORTED.- Ensure the PacketEvents enum constant you reference matches what’s available in your pinned dependency.
23-24: The definition ofcustomModelDataComponentisn’t found in the grep results—noval ItemMeta.customModelDataComponentdeclaration, and usages everywhere assume it’s non-null. Likely it’s provided by your Paper API or an imported extension with a non-nullable return. Since we can’t locate its declaration in this codebase, please verify in the API stubs (or your imported extension) thatcustomModelDataComponentis declared non-nullable; if it’s actually nullable, add a null-check or default-construct guard here.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/ColorsCustomModelData.kt (3)
13-18: Solid data model and annotation usageThe type id, icon, and default value are sensible. Data class choice and sealed type integration look good.
24-31: Meta update pattern looks correctGood use of editMeta and no-op when unchanged; avoids unnecessary writes.
20-23: Support boundary confirmed: 1.21.4+
CustomModelDataComponent.colors was introduced in Paper 1.21.4, so the existing guard is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongArrayPdcData.kt (3)
16-18: Avoid repeated boxing/copies by storing LongArray directly (optional)List incurs boxing and toLongArray() allocates each time. If configs don’t rely on List, consider holding LongArray.
Apply:
@AlgebraicTypeInfo("long_array", Colors.PURPLE, "fa6-solid:database") data class LongArrayPdcData( - val value: List<Long> = emptyList() + val value: LongArray = longArrayOf() ) : PdcDataType { - private fun toLongArray(): LongArray = - value.toLongArray() + private fun toLongArray(): LongArray = valueAnd adjust call sites below to use
valuedirectly where applicable.
19-33: Skip no-op writes to reduce meta churn (optional)Avoid setting/removing when nothing changes; helps prevent unnecessary item meta updates.
) { item.editMeta { meta -> val container = meta.persistentDataContainer - if (value.isEmpty()) { - container.remove(key) - } else { - container.set(key, PersistentDataType.LONG_ARRAY, toLongArray()) - } + val expected = toLongArray() + val current = container.get(key, PersistentDataType.LONG_ARRAY) + when { + expected.isEmpty() -> if (container.has(key, PersistentDataType.LONG_ARRAY)) { + container.remove(key) + } + current == null || !current.contentEquals(expected) -> { + container.set(key, PersistentDataType.LONG_ARRAY, expected) + } + else -> Unit + } } }
35-49: Minor: early-return fast path and clearer empty semanticsSmall readability/micro-allocation win; also makes the “empty expected” case explicit.
): Boolean { - val expected = toLongArray() - val container = item.itemMeta?.persistentDataContainer - ?: return expected.isEmpty() - val actual = container.get(key, PersistentDataType.LONG_ARRAY) - return when { - actual == null -> expected.isEmpty() - else -> actual.contentEquals(expected) - } + if (value.isEmpty()) { + val container = item.itemMeta?.persistentDataContainer ?: return true + val actual = container.get(key, PersistentDataType.LONG_ARRAY) ?: return true + return actual.isEmpty() + } + val expected = toLongArray() + val container = item.itemMeta?.persistentDataContainer ?: return false + val actual = container.get(key, PersistentDataType.LONG_ARRAY) ?: return false + return actual.contentEquals(expected) }If your intended semantics are “empty expected means key absent OR empty array, but not mismatched type,” this holds. If you want “mismatched type should never match,” change the first return-true path to also check
!container.has(key, PersistentDataType.LONG_ARRAY).engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt (2)
17-29: Skip unnecessary meta clone; also verify main-thread usage
- micro: Read current value before
editMetato avoid cloning/writing when unchanged.- correctness: Bukkit/Paper item/meta APIs are not thread-safe; ensure callers invoke
applyon the primary server thread.Apply this diff:
- ) { - item.editMeta { meta -> - val pdc = meta.persistentDataContainer - if (pdc.get(key, PersistentDataType.LONG) != value) { - pdc.set(key, PersistentDataType.LONG, value) - } - } - } + ) { + val current = item.itemMeta?.persistentDataContainer?.get(key, PersistentDataType.LONG) + if (current != value) { + item.editMeta { meta -> + meta.persistentDataContainer.set(key, PersistentDataType.LONG, value) + } + } + }
18-22: Silence unused parameter warnings for clarityThese parameters are unused by design; prefix with
_(or add@Suppress("UNUSED_PARAMETER")) to make intent explicit.- player: Player?, - interactionContext: InteractionContext?, + _player: Player?, + _interactionContext: InteractionContext?, @@ - player: Player?, - interactionContext: InteractionContext?, + _player: Player?, + _interactionContext: InteractionContext?,Also applies to: 32-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/ColorsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/FloatsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/LegacyCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/StringsCustomModelData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BooleanPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ByteArrayPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BytePdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/DoublePdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/FloatPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntArrayPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongArrayPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ShortPdcData.kt(1 hunks)engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/StringPdcData.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/FloatPdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BooleanPdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ByteArrayPdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntPdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/DoublePdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/ColorsCustomModelData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/StringsCustomModelData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/BytePdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/LegacyCustomModelData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/ShortPdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/IntArrayPdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/StringPdcData.kt
- engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/customModelDataTypes/FloatsCustomModelData.kt
🔇 Additional comments (3)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongArrayPdcData.kt (1)
11-15: Clean, idiomatic PDC data-type implementation — LGTMAnnotation, defaults, and interface wiring look solid. Semantics (remove-on-empty, set-otherwise) align with typical PDC usage.
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/pdcTypes/LongPdcData.kt (2)
11-14: Long end-to-end implementation — correct and consistentUsing
Longin the API andPersistentDataType.LONGfor storage fixes the earlier truncation concern and aligns type semantics throughout. Good data-class usage for equality/hash.
31-39: Null-safe equality check is correctComparing the fetched
Long?tovaluehandles absence vs. equality cleanly.
...er/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemModelComponent.kt
Show resolved
Hide resolved
...per/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemNameComponent.kt
Outdated
Show resolved
Hide resolved
...aper/src/main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemPDCComponent.kt
Outdated
Show resolved
Hide resolved
...main/kotlin/com/typewritermc/engine/paper/utils/item/components/ItemTooltipStyleComponent.kt
Outdated
Show resolved
Hide resolved
|
i think these are resolved |
|
Sorry btw that i was so fast gone in the call, there was a private emergency i had |
3 participants
Adds several ItemComponent:
CustomModelData
JukeboxPlayable
PersistentDataContainer
TooltipStyle
Summary by CodeRabbit