Improve path stream documentation #351
Conversation
WalkthroughThis update introduces new documentation and Kotlin code for the "Path Streams" system, which visually guides players to dynamic locations using path streams. It adds detailed developer documentation, Gradle build dependencies, and example Kotlin entries and producers for displaying and customizing path streams within a road network extension. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant AudienceEntry (Single/Multi)
participant PathStreamDisplayEntry
participant PathStreamProducer
participant PathStreamDisplay
Player->>AudienceEntry: Request path stream display
AudienceEntry->>PathStreamDisplayEntry: Create display instance
PathStreamDisplayEntry->>PathStreamProducer: Generate path stream (with start/end positions)
PathStreamProducer-->>PathStreamDisplayEntry: Return PathStream
PathStreamDisplayEntry->>PathStreamDisplay: Render path stream for Player
PathStreamDisplay-->>Player: Visualize path (e.g., particles)
Poem
✨ Finishing Touches
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 (
|
gabber235
force-pushed
the
codex/create-developer-documentation-for-pathstreamdisplayentries
branch
from
4df7064 to
11df7b7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (11)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/_category_.yml (1)
1-1: Add explicit ordering for sidebar
The new category file correctly defineslabel: Path Streams. Consider adding apositionfield if you need explicit ordering of categories in the sidebar.engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/entry/AudienceManager.kt (1)
223-226: Consider an inline reified overload fordescendants
The new extension is useful but still requires passing aKClass. You could add ainline fun <reified E : AudienceEntry> AudienceFilterEntry.descendants(): List<Ref<E>>to let callers simply writedescendants<MyEntry>().documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx (2)
22-22: Fix missing comma after introductory phrase.Static analysis correctly identifies a missing comma that affects readability.
-stream and renders its positions. In practice you only override the methods that +stream and renders its positions. In practice, you only override the methods that🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: A comma is probably missing here.
Context: ...at stream and renders its positions. In practice you only override the methods that crea...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
43-43: Fix missing comma in conditional clause.Static analysis correctly identifies a missing comma that improves sentence flow.
-When you need more advanced behaviour implement your own `PathStreamProducer` +When you need more advanced behaviour, implement your own `PathStreamProducer`🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: A comma might be missing here.
Context: ... Producers When you need more advanced behaviour implement your ownPathStreamProducer...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/ParticlePathStreamDisplay.kt (1)
19-19: Renamespeedparameter toparticleSpeedfor clarity.The parameter name
speedis ambiguous in the context of path streams. Consider renaming it toparticleSpeedto maintain consistency with other display entries and avoid confusion with travel speed.- private val speed: Float = 20f + private val particleSpeed: Float = 20fAnd update line 27:
- speed, + particleSpeed,extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamBasics.kt (1)
19-24: Consider using a more realistic example for documentation.Using
Position.ORIGINas theendPositionmight be confusing in documentation examples. Consider using a more realistic position calculation to better demonstrate the intended usage pattern.override suspend fun display(): AudienceDisplay = SinglePathStreamDisplay( road, - { display }, - endPosition = { Position.ORIGIN } + { display }, // Lambda allows dynamic display selection + endPosition = { player -> player.location.toPosition() + Vector(10, 0, 10) } // Example: 10 blocks away )extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/LinePathStream.kt (1)
52-56: Remove unnecessary startTime validation.The
startTime >= 0check is redundant sincestartTimeis initialized toSystem.currentTimeMillis()in the parentPathStreamclass, which always returns a positive value.init { require(path.isNotEmpty()) { "Path must not be empty" } - require(startTime >= 0) { "Start time must not be negative" } require(speed > 0) { "Speed must be positive" } }extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamDisplayExample.kt (1)
16-23: Add comment to empty dispose method.While an empty
dispose()method is acceptable for this example, adding a comment would improve clarity and address the static analysis warning.- override fun dispose() {} + override fun dispose() { + // No resources to clean up in this example + }🧰 Tools
🪛 detekt (1.23.8)
[warning] 21-21: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/PathStreamDisplay.kt (2)
111-111: Remove unnecessary empty line.val (stream, display) = value - if (stream.ref.id == display.id) {
137-151: Consider renaming class to avoid confusion.The name
StreamProduceris misleading since it's not actually a producer but rather a configuration object that createsPathStreamProducerinstances. This could confuse developers about its purpose.Consider renaming to
StreamProducerConfigorPathStreamConfig:-class StreamProducer( +class PathStreamConfig( val id: String, val ref: Ref<PathStreamDisplayEntry>, val startPosition: (Player) -> Position = Player::position, val endPosition: (Player) -> Position, ) { fun createProducer(roadNetwork: Ref<RoadNetworkEntry>, player: Player): PathStreamProducer { return ref.get()?.createProducer(player, roadNetwork, startPosition, endPosition) ?: LinePathStreamProducer( player = player, roadNetwork = roadNetwork, startPosition = startPosition, endPosition = endPosition ) } }extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/audience/PathFindingPathStream.kt (1)
73-74: Consider explicit bounds checking instead of coerceIn.Using
coerceInsilently clamps the index, which could hide bugs wherecurrentEdgeIndexexceeds the valid range. Consider explicit bounds checking to catch programming errors early.- private val currentEdge: GPSEdge - get() = edges[currentEdgeIndex.coerceIn(0 until edges.size)] + private val currentEdge: GPSEdge + get() { + require(currentEdgeIndex in edges.indices) { "Current edge index $currentEdgeIndex is out of bounds [0, ${edges.size})" } + return edges[currentEdgeIndex] + }
🛑 Comments failed to post (7)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/LocationObjectivesPathStream.kt (2)
38-40:
⚠️ Potential issueConsider thread safety for lazy initialization.
Using
LazyThreadSafetyMode.NONEcould lead to race conditions ifdisplay()is called from multiple threads. Consider using the default thread-safe mode unless you're certain this will only be accessed from a single thread.- private val objectiveDisplays: Map<Ref<LocationObjectiveEntry>, List<Ref<PathStreamDisplayEntry>>> by lazy( - LazyThreadSafetyMode.NONE - ) { + private val objectiveDisplays: Map<Ref<LocationObjectiveEntry>, List<Ref<PathStreamDisplayEntry>>> by lazy {🤖 Prompt for AI Agents
In extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/LocationObjectivesPathStream.kt around lines 38 to 40, the lazy initialization uses LazyThreadSafetyMode.NONE which is not thread-safe and may cause race conditions if accessed concurrently. Change the lazy initialization to use the default thread-safe mode by removing the explicit LazyThreadSafetyMode.NONE parameter to ensure safe concurrent access.
38-40:
⚠️ Potential issueUse thread-safe lazy initialization mode.
The
LazyThreadSafetyMode.NONEis not thread-safe and could cause race conditions when multiple players access theobjectiveDisplaysmap simultaneously.Apply this diff to fix the thread safety issue:
- private val objectiveDisplays: Map<Ref<LocationObjectiveEntry>, List<Ref<PathStreamDisplayEntry>>> by lazy( - LazyThreadSafetyMode.NONE - ) { + private val objectiveDisplays: Map<Ref<LocationObjectiveEntry>, List<Ref<PathStreamDisplayEntry>>> by lazy {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private val objectiveDisplays: Map<Ref<LocationObjectiveEntry>, List<Ref<PathStreamDisplayEntry>>> by lazy { // ... existing initialization logic ... }🤖 Prompt for AI Agents
In extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/LocationObjectivesPathStream.kt around lines 38 to 40, the lazy initialization of the objectiveDisplays map uses LazyThreadSafetyMode.NONE, which is not thread-safe. Change the lazy initialization mode to LazyThreadSafetyMode.SYNCHRONIZED to ensure thread safety when multiple players access the map concurrently.extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/audience/PathFindingPathStream.kt (1)
87-103:
⚠️ Potential issuePotential edge index issue in navigation logic.
After incrementing
currentEdgeIndexat line 93, accessingcurrentEdgeat line 97 could throw an exception if we've reached the last edge, as the getter will try to accessedges[edges.size]which is out of bounds.Consider storing the next edge before incrementing the index:
if (!navigator.isComplete()) return position +val nextEdge = if (currentEdgeIndex + 1 < edges.size) edges[currentEdgeIndex + 1] else currentEdge currentEdgeIndex++ if (!shouldContinue()) return position navigator = NavigationActivityTaskState.Walking( roadNetwork, - currentEdge, + nextEdge, position.toProperty(), speed.toFloat(), rotationLookAhead = 0, )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.override fun forwardPath(): Position { require(currentEdgeIndex < edges.size) { "No more edges to walk on" } navigator.tick(IndividualActivityContext(emptyRef(), player, isViewed = true)) val position = navigator.position().toPosition() if (!navigator.isComplete()) return position val nextEdge = if (currentEdgeIndex + 1 < edges.size) edges[currentEdgeIndex + 1] else currentEdge currentEdgeIndex++ if (!shouldContinue()) return position navigator = NavigationActivityTaskState.Walking( roadNetwork, nextEdge, position.toProperty(), speed.toFloat(), rotationLookAhead = 0, ) return position }🤖 Prompt for AI Agents
In extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/audience/PathFindingPathStream.kt between lines 87 and 103, the code increments currentEdgeIndex before accessing currentEdge, which risks an out-of-bounds exception if at the last edge. To fix this, store the current edge in a local variable before incrementing currentEdgeIndex, then use that stored edge when creating the new NavigationActivityTaskState.Walking instance, ensuring safe access without exceeding the edges list bounds.extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamDisplayExample.kt (1)
26-34:
⚠️ Potential issueFix potential index out of bounds in forwardPath().
The
forwardPath()method increments the index without checking bounds, which could cause anArrayIndexOutOfBoundsExceptionif called whenindex >= points.size.class ExamplePathStream( displays: List<PathStreamDisplay>, private val points: List<Position>, ) : PathStream(displays) { private var index = 0 - override fun forwardPath(): Position = points[index++] + override fun forwardPath(): Position { + val currentIndex = index + if (currentIndex < points.size) { + index++ + } + return points[currentIndex.coerceIn(0, points.size - 1)] + } override fun shouldContinue(): Boolean = index < points.size }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.class ExamplePathStream( displays: List<PathStreamDisplay>, private val points: List<Position>, ) : PathStream(displays) { private var index = 0 override fun forwardPath(): Position { val currentIndex = index if (currentIndex < points.size) { index++ } return points[currentIndex.coerceIn(0, points.size - 1)] } override fun shouldContinue(): Boolean = index < points.size } //</code-block:custom_path_stream>🤖 Prompt for AI Agents
In extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamDisplayExample.kt between lines 26 and 34, the forwardPath() method increments the index and accesses points[index] without checking if index is within bounds, risking an ArrayIndexOutOfBoundsException. To fix this, add a check before accessing points[index] to ensure index is less than points.size, and handle the case when index is out of bounds appropriately, such as throwing a controlled exception or returning a default value.extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/PathStreamDisplay.kt (2)
254-268: 🛠️ Refactor suggestion
⚠️ Potential issueReplace Unconfined dispatcher and reduce timeout duration.
Using
Dispatchers.Unconfinedin production code can lead to unpredictable behavior and potential stack overflow issues. Additionally, a 5-second timeout for tick operations is excessive and could cause performance degradation.Use a proper dispatcher and reduce the timeout:
- Dispatchers.Unconfined.launch { + Dispatchers.Default.launch { mutex.lock() try { var writeIndex = 0 for (readIndex in 0..streams.lastIndex) { val stream = streams[readIndex] - val retain = withTimeoutOrNull(5.seconds) { + val retain = withTimeoutOrNull(100) { // 100ms should be sufficient for a tick operation try { stream.tick() } catch (e: Throwable) { e.printStackTrace() false } } ?: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Dispatchers.Default.launch { mutex.lock() try { var writeIndex = 0 for (readIndex in 0..streams.lastIndex) { val stream = streams[readIndex] val retain = withTimeoutOrNull(100) { // 100ms should be sufficient for a tick operation try { stream.tick() } catch (e: Throwable) { e.printStackTrace() false } } ?: false if (!retain) {🤖 Prompt for AI Agents
In extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/PathStreamDisplay.kt between lines 254 and 268, replace Dispatchers.Unconfined with a more appropriate dispatcher like Dispatchers.Default to avoid unpredictable behavior and stack overflow risks. Also, reduce the timeout duration in withTimeoutOrNull from 5 seconds to a shorter interval (e.g., 500 milliseconds) to improve performance and responsiveness during tick operations.
67-89:
⚠️ Potential issueRemove redundant producer ticking to prevent double execution.
The
tick()method currently ticks producers twice - once for each player in the loop (lines 68-85) and then again for all producers (lines 86-88). This causes duplicate ticking for the same producers.Remove the redundant iteration:
override fun tick() { players.forEach { player -> producers.computeIfPresent(player.uniqueId) { _, producer -> val entry = this.display(player).get() val id = entry?.id ?: "" if (producer.id == id) { producer.tick() return@computeIfPresent producer } producer.dispose() (entry?.createProducer(player, roadNetwork, startPosition, endPosition) ?: LinePathStreamProducer( player = player, roadNetwork = roadNetwork, startPosition = startPosition, endPosition = endPosition )).apply { tick() } } } - producers.values.forEach { - it.tick() - } }🤖 Prompt for AI Agents
In extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/PathStreamDisplay.kt between lines 67 and 89, the tick() method calls tick() on producers twice: once inside the players.forEach loop and again in the producers.values.forEach loop. To fix this, remove the second iteration over producers.values that calls tick(), ensuring each producer is ticked only once during the players.forEach loop.extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/displays/DustPathStreamDisplay.kt (1)
58-73: 🛠️ Refactor suggestion
Consider extracting common
createProducerimplementation to reduce duplication.The
createProducermethod is identical acrossTotemPathStreamDisplay,FlamePathStreamDisplay, andDustPathStreamDisplay. This violates the DRY principle.Consider creating a base class or extension function to share this common implementation:
abstract class BaseParticlePathStreamDisplayEntry : PathStreamDisplayEntry { override fun createProducer( player: Player, roadNetwork: Ref<RoadNetworkEntry>, startPosition: (Player) -> Position, endPosition: (Player) -> Position ) = LinePathStreamProducer( player, ref(), roadNetwork, startPosition, endPosition, refreshDuration, travelSpeed, displays(), ) }🤖 Prompt for AI Agents
In extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/displays/DustPathStreamDisplay.kt around lines 58 to 73, the createProducer method is duplicated in multiple classes. To fix this, create a new abstract base class (e.g., BaseParticlePathStreamDisplayEntry) that implements the common createProducer method, then have DustPathStreamDisplay, TotemPathStreamDisplay, and FlamePathStreamDisplay inherit from this base class to reuse the implementation and eliminate duplication.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx (2)
22-22: Add missing comma for better readability.The sentence would benefit from a comma after the introductory phrase for improved clarity.
-stream and renders its positions. In practice you only override the methods that +stream and renders its positions. In practice, you only override the methods that🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: A comma is probably missing here.
Context: ...at stream and renders its positions. In practice you only override the methods that crea...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
43-43: Add missing comma for grammatical correctness.The sentence needs a comma after the introductory clause for proper grammar.
-When you need more advanced behaviour implement your own `PathStreamProducer` +When you need more advanced behaviour, implement your own `PathStreamProducer`🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Possible missing comma found.
Context: ... Producers When you need more advanced behaviour implement your ownPathStreamProducer...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/_category_.yml(1 hunks)documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx(1 hunks)documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx(1 hunks)extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamBasics.kt(1 hunks)extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamDisplayExample.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx
- documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/category.yml
- extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamBasics.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamDisplayExample.kt (1)
extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/PathStreamDisplay.kt (1)
calculatePathing(296-302)
🪛 LanguageTool
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx
[uncategorized] ~22-~22: A comma is probably missing here.
Context: ...at stream and renders its positions. In practice you only override the methods that crea...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[uncategorized] ~43-~43: Possible missing comma found.
Context: ... Producers When you need more advanced behaviour implement your own PathStreamProducer...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 detekt (1.23.8)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamDisplayExample.kt
[warning] 21-21: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (6)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx (1)
1-47: Excellent documentation structure and content.The documentation effectively explains the PathStreamDisplayEntry concept with clear use cases, visual diagrams, and practical examples. The structure flows logically from basic concepts to advanced implementations.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: A comma is probably missing here.
Context: ...at stream and renders its positions. In practice you only override the methods that crea...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[uncategorized] ~43-~43: Possible missing comma found.
Context: ... Producers When you need more advanced behaviour implement your ownPathStreamProducer...(AI_HYDRA_LEO_MISSING_COMMA)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/PathStreamDisplayExample.kt (5)
16-22: Simple and correct particle display implementation.The implementation correctly spawns flame particles at the specified position. The empty
dispose()method is appropriate for this simple example since no cleanup is required.🧰 Tools
🪛 detekt (1.23.8)
[warning] 21-21: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
26-33: Well-implemented path stream with proper bounds checking.The implementation correctly manages the position index and provides proper bounds checking in
shouldContinue(). The sequential iteration through points is appropriate for the example.
37-67: Properly configured entry class with sensible defaults.The entry class is correctly annotated and provides reasonable default values. The refresh duration of 1700ms and speed of 20.0 are appropriate for demonstration purposes.
70-94: Solid custom producer implementation following established patterns.The producer correctly extends the base class and implements the abstract
refreshPath()method. The error handling for null paths and empty lines is appropriate, and the use offlatten()to convert nested path lists to a single line is correct.
88-93:✅ Verification successful
Verify error handling in custom producer implementation.
The implementation correctly handles the case where
calculatePathing()returns null, but consider verifying that the empty line case (line 91) properly handles edge cases in path calculation.
🏁 Script executed:
#!/bin/bash # Description: Check for other usages of calculatePathing() to understand expected behavior # Expected: Find similar null-checking patterns in the codebase ast-grep --pattern 'calculatePathing()' | head -20 rg -A 5 -B 5 'calculatePathing\(\)' --type kotlinLength of output: 5081
Empty path handling is consistent with the codebase
Both the example and production implementations use the same pattern—returningnullwhenpaths.flatten()yields an empty list—so no additional error handling is required here.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt (1)
97-97: Improve comment clarity.The comment about "sub functions" is awkward and unclear.
- // Create a path stream which is used to keep track of the current position and forwards for displaying. - // Typewriter will automatically trigger the sub functions for you. + // Create a path stream to track the current position and forward it to displays. + // Typewriter will automatically call the stream methods during rendering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx(1 hunks)documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx(1 hunks)extensions/_DocsExtension/build.gradle.kts(2 hunks)extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt(1 hunks)extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
extensions/_DocsExtension/build.gradle.kts (1)
module-plugin/api/src/main/kotlin/com/typewritermc/moduleplugin/TypewriterModuleConfiguration.kt (5)
dependencies(106-108)dependencies(174-181)dependencies(198-209)dependency(178-180)dependency(206-208)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt (1)
engine/engine-core/src/main/kotlin/com/typewritermc/core/entries/EntryReference.kt (1)
emptyRef(6-6)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt (1)
extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/PathStreamDisplay.kt (3)
calculatePathing(296-302)points(304-313)findPaths(332-343)
🪛 LanguageTool
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx
[grammar] ~10-~10: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...ing a PathStream There are two ways to setup a PathStream. It depends on how many ...
(NOUN_VERB_CONFUSION)
[style] ~11-~11: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...epends on how many target positions you want to show. ### Single PathStream If you on...
(REP_WANT_TO_VB)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx
[grammar] ~17-~17: Do not use the modal verb ‘must’ after the verb ‘be’. Did you mean “must”?
Context: ...c Structure A PathStreamDisplayEntry is must expose two functions: - `createProduce...
(BE_MD)
[uncategorized] ~30-~30: A comma is probably missing here.
Context: ...at stream and renders its positions. In practice you only override the methods that crea...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Documentation Build
- GitHub Check: build
🔇 Additional comments (1)
extensions/_DocsExtension/build.gradle.kts (1)
2-4: LGTM! Dependencies properly configured.The dependencies are correctly added to support the new path stream documentation examples:
compileOnlydependency on RoadNetworkExtension allows compilation without runtime dependency- Typewriter dependency declaration follows the proper format
Also applies to: 20-23
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx
Outdated
Show resolved
Hide resolved
...mentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx
Outdated
Show resolved
Hide resolved
...main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt
Show resolved
Hide resolved
...ion/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt
Outdated
Show resolved
Hide resolved
gabber235
force-pushed
the
codex/create-developer-documentation-for-pathstreamdisplayentries
branch
from
d94d876 to
bb7ce9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx (1)
19-20: Clarify phrasing to avoid repetition
The sentence “Customize this if you want to modify the path stream’s positioning.” is similar to nearby wording. Consider rephrasing for variety and clarity, for example:Override the producer to adjust the stream positioning.🧰 Tools
🪛 LanguageTool
[style] ~20-~20: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... being displayed. Customize this if you want to modify the path stream's positioning. ...(REP_WANT_TO_VB)
documentation/plugins/code-snippets/snippets.json (1)
158-161: Typo in inline comment
In thecustom_path_stream_producersnippet:// separate our the different parts of the path finding.should read:
- // separate our the different parts of the path finding. + // separate out the different parts of the path finding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx(1 hunks)documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx(1 hunks)documentation/plugins/code-snippets/snippets.json(1 hunks)extensions/_DocsExtension/build.gradle.kts(2 hunks)extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt(1 hunks)extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx
- extensions/_DocsExtension/build.gradle.kts
- extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt
🧰 Additional context used
🪛 LanguageTool
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx
[style] ~20-~20: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... being displayed. Customize this if you want to modify the path stream's positioning. ...
(REP_WANT_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Documentation Build
- GitHub Check: build
🔇 Additional comments (1)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx (1)
1-1:⚠️ Potential issueMissing MDX frontmatter for Docusaurus page
This file lacks the required frontmatter (e.g.,id,title,sidebar_label) at the top, so Docusaurus won’t render it in the sidebar.⛔ Skipped due to learnings
Learnt from: Marten-Mrfc PR: gabber235/Typewriter#333 File: documentation/docs/docs/03-creating-stories/04-entity-extension/04-activities.mdx:1-3 Timestamp: 2025-05-11T07:03:22.666Z Learning: In the Typewriter project's documentation MDX files, minimal front matter that includes only the `difficulty` field is acceptable. Additional fields like `title`, `id`, or `sidebar_label` are not required even if the heading is present in the document body.
gabber235
force-pushed
the
codex/create-developer-documentation-for-pathstreamdisplayentries
branch
from
bb7ce9f to
6e4377e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt (1)
82-91:⚠️ Potential issueDuplicate path-finding logic & typo persist
The block:val (_edges, _paths) = calculatePathing() ?: return null // … then re-implements the same stepsstill duplicates
calculatePathing()and contains the typo “separate our”. Re-use the helper or delete the first call, and fix the comment:-// separate our the different parts of the path finding. +// separate out the different parts of the path-finding. -val (_edges, _paths) = calculatePathing() ?: return nullSee prior review; nothing changed.
documentation/plugins/code-snippets/snippets.json (1)
142-165: 🛠️ Refactor suggestionIncorrect file paths still unresolved
Snippet entries still point to
"src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/…",
but the actual sources live underextensions/_DocsExtension/….Example fix:
-"file": "src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt", +"file": "extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt",Apply to
single_path_stream_display,multi_path_stream_display,path_stream_display_entry,custom_path_stream_display,custom_path_stream_producer, andcustom_path_stream.Unfixed paths will break snippet extraction.
🧹 Nitpick comments (2)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx (1)
19-21: Tighten wording to avoid repetition“… being displayed. Customize this if you want to modify the path stream’s positioning.” – the “want to” phrasing is repeated twice in close proximity.
Consider re-phrasing for a crisper read:-Customize this if you want to modify the path stream's positioning. +Customize it to adjust the path stream’s positioning.🧰 Tools
🪛 LanguageTool
[style] ~20-~20: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... being displayed. Customize this if you want to modify the path stream's positioning. ...(REP_WANT_TO_VB)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt (1)
32-48: Use your custom ExampleProducer instead of LinePathStreamProducer
ExampleProduceris defined below but never referenced –createProducerinstantiatesLinePathStreamProducer, so the example of a custom producer is effectively dead code.- LinePathStreamProducer( - player = player, - ref = ref(), - roadNetwork = roadNetwork, - startPosition = startPosition, - endPosition = endPosition, - refreshDuration = refreshDuration, - speed = travelSpeed, - displayEntries = displays(), - ) + ExampleProducer( + player = player, + id = ref().id, // or another stable id + roadNetwork = roadNetwork, + startPosition = startPosition, + endPosition = endPosition, + refreshDuration = refreshDuration, + displayEntries = displays(), + )This keeps the sample cohesive and showcases the custom producer in action.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx(1 hunks)documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx(1 hunks)documentation/plugins/code-snippets/snippets.json(1 hunks)extensions/_DocsExtension/build.gradle.kts(2 hunks)extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt(1 hunks)extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/index.mdx
- extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamBasics.kt
- extensions/_DocsExtension/build.gradle.kts
🧰 Additional context used
🧬 Code Graph Analysis (1)
extensions/_DocsExtension/src/main/kotlin/com/typewritermc/example/entries/manifest/pathstream/PathStreamDisplayExample.kt (1)
extensions/RoadNetworkExtension/src/main/kotlin/com/typewritermc/roadnetwork/entries/PathStreamDisplay.kt (3)
calculatePathing(296-302)points(304-313)findPaths(332-343)
🪛 LanguageTool
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx
[style] ~20-~20: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... being displayed. Customize this if you want to modify the path stream's positioning. ...
(REP_WANT_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Documentation Build
- GitHub Check: build
🔇 Additional comments (1)
documentation/docs/develop/02-extensions/04-entries/manifest/path_stream/path_stream_display.mdx (1)
36-38: Verify relative path to snippets.jsonThe import
<CodeSnippet … json={require("../../../../snippets.json")} />goes only four levels up, which lands in
02-extensions/, not indocumentation/plugins/code-snippets/. Unless a custom webpack alias rewires this, Docusaurus will fail to resolve the module.
Double-check the path or switch to an absolute alias such as:- json={require("../../../../snippets.json")} + json={require("@site/documentation/plugins/code-snippets/snippets.json")}
gabber235
deleted the
codex/create-developer-documentation-for-pathstreamdisplayentries
branch
1 participant
Summary
Testing
npm run test(fails: docusaurus not found)./gradlew :_DocsExtension:build(no output - build couldn't run)https://chatgpt.com/codex/tasks/task_e_6847efd735188322982f9d2ce8c301cb
Summary by CodeRabbit
New Features
Documentation