[1.20.4] Feat: New gui, texture api and buffer api #98
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
common/src/main/kotlin/com/lambda/graphics/buffer/VertexPipeline.kt (1)
188-190: 🛠️ Refactor suggestion** Implement proper OpenGL resource cleanup in finalize().**
This empty method is currently unutilized. Please remove it if it’s unnecessary or perform resource cleanup (e.g., deleting VAO/VBO/EBO) to avoid GPU memory leaks. Also, relying on finalization for cleanup is discouraged in Kotlin; an explicit
close()method orAutoCloseableimplementation is typically more reliable.🧰 Tools
🪛 detekt (1.23.7)
[warning] 188-190: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🧹 Nitpick comments (27)
common/src/main/resources/assets/lambda/shaders/fragment/signed_distance_field.frag (3)
1-10: Check constant naming: SPHREAD vs SPREADThe constant name
SPHREADon line 9 appears to be a typo of "SPREAD" which would be more appropriate for a blur radius constant.-#define SPHREAD 4 +#define SPREAD 4
27-28: Add protection against division by zeroWhile unlikely due to the nature of the exp function, it's still good practice to add protection against division by zero.
- color = colors / blurWeight; + color = colors / max(blurWeight, vec4(0.0001));
1-29: Add documentation about the shader purpose and parametersThis shader implements a specific blur technique for signed distance fields, but lacks documentation explaining its purpose and how the parameters affect the output.
Consider adding comments to explain:
- The purpose of the shader (SDF blurring/smoothing)
- How
u_TexelSizeshould be calculated- How the weight calculation works and what it achieves
- The expected input texture format (presumably a distance field)
common/src/main/kotlin/com/lambda/gui/component/window/AnimatedWindowChild.kt (2)
45-58: Ensure animation logic is tested thoroughly.The code uses exponential animations (
animation.exp) and dynamically adjusts the animation speed based on theindexandlastIndex. Consider adding unit or integration tests to validate that:
- The
showAnimationcorrectly transitions between 0.0 and 1.0.- The computed
speedvalues yield the intended user experience for sequential animations.
75-81: Consider consolidating initialization blocks.Having two separate
initblocks controlling aspects of the title bar text updates can introduce complexity. You might merge them into a single initialization flow or comment explicitly on each block’s purpose, making it clearer for future maintainers.common/src/main/kotlin/com/lambda/graphics/buffer/vertex/ElementBuffer.kt (1)
26-29: Document usage parameter.Including a brief comment clarifying the intended usage for
usage: Int = GL_DYNAMIC_DRAWwould help maintainers understand why dynamic draw is used here (versus static or stream). Doing so also clarifies the performance trade-offs of changing this parameter in the future.common/src/main/kotlin/com/lambda/gui/component/core/UIBuilder.kt (1)
33-42: Robust layout insertion method with proper error handling.The
insertLayoutfunction provides a clean way to insert layouts with appropriate error checking. The check for base layout ownership prevents invalid insertions.Consider adding KDoc to explain the function's purpose and parameters, especially the meaning of the
nextparameter, to improve code readability.+/** + * Inserts this layout into the children of the specified owner layout. + * + * @param owner The layout that will contain this layout + * @param base The reference layout in the owner's children list + * @param next If true, inserts after the base layout; if false, inserts before it + * @throws IllegalStateException If the base layout doesn't belong to the specified owner + * @return This layout (for chaining) + */ fun <T : Layout> T.insertLayout( owner: Layout, base: Layout, next: Boolean ) = apply {common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleWindow.kt (2)
44-55: Reduce repeated code paths
BothonWindowExpandandonWindowMinimizesetisMinimizedon the same module collection. Consider consolidating this functionality to avoid duplicated logic.- onWindowExpand { - modules.forEach { - it.isMinimized = true - } - } - - onWindowMinimize { - modules.forEach { - it.isMinimized = true - } - } + val minimizeAll: () -> Unit = { + modules.forEach { it.isMinimized = true } + } + onWindowExpand(minimizeAll) + onWindowMinimize(minimizeAll)
57-69: Centralize window creation steps
ThemoduleWindowextension function is convenient, but if you plan to create windows with shared settings or common behavior, consider factoring out repeated initialization to ensure maintainability.common/src/main/kotlin/com/lambda/gui/impl/clickgui/SettingLayout.kt (1)
51-53: Question default minimization
By settingisMinimized = true, you're collapsing the layout at creation time. Confirm if this meets UX expectations. If not, you may need to invert or remove this behavior.common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/EnumSelector.kt (2)
28-28: Address the slider TODO
The comment suggests planning to morph this into a slider or completing the implementation. Let me know if you'd like a preliminary approach or a new issue to track the requirement.
44-45: Use friendlier labels
Currently, you’re displayingenumEntry.nameverbatim. For improved UX, consider allowing a custom or localized display name if one is available.common/src/main/kotlin/com/lambda/module/hud/TickShiftCharge.kt (2)
61-63: Avoid negative corner radii
IfClickGui.roundRadiusis smaller thanpadding, subtracting them may yield a negative radius and cause wonky rendering.Consider the following tweak:
- roundRadius = ClickGui.roundRadius - padding, + roundRadius = max(0.0, ClickGui.roundRadius - padding),
68-76: Conditional outline rendering
Drawing the outline only whenClickGui.outlineis true is a clean approach. Exposing a separate parameter for the shading might provide additional user customization.common/src/main/kotlin/com/lambda/gui/component/window/WindowContent.kt (1)
68-117: Scroll and rubberband effect
Implementing a rubberband effect enhances UX. The factor10.0fordwheelis a magic number; consider making this scroll sensitivity configurable.common/src/main/kotlin/com/lambda/gui/component/window/Window.kt (1)
40-50: Window class constructor
Offers a comprehensive set of adjustable parameters. Consider moving defaults (e.g.,initialSize,title) to a config or constants for future flexibility.common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleLayout.kt (4)
53-59: Consider replacing the manual hover timestamp with a timer or animation controller.You already have a
TODOcomment suggesting this improvement. Relying onSystem.currentTimeMillis()can become cumbersome and harder to manage. A more elegant approach is to use a timer-based mechanism or unify it with your existing animation logic for consistent updates.Would you like me to propose a simple timer-based approach for measuring hover duration?
60-117: Potential performance concern for repeated color calculations.Each frame, multiple operations occur (lerping, alpha calculation, color blending). While this is functional, consider caching intermediate color states when they are unlikely to change frequently (e.g., whenever
module.enabledchanges) to reduce overhead and repeated color computations, especially on lower-spec systems.
155-168: Possible confusion when minimizing other modules on expand.In
onWindowExpand, all otherModuleLayoutinstances are minimized. This behavior might confuse users expecting multiple modules to be simultaneously expanded. Confirm user expectations before finalizing this. If intended, a comment explaining the rationale will help maintainers.
191-239: Clarify function usage inbackgroundTint.Although you’ve mentioned "Not for external usage," it’s declared as a public function in a companion object. If truly internal, consider reducing visibility or adding a note that it’s intended for internal composition only, to prevent unintended usage outside the module.
common/src/main/kotlin/com/lambda/gui/component/core/TextField.kt (1)
53-58: Optimize rendering alignment calculations.Repeatedly calling
lerpfor horizontal and vertical alignment insideonRenderis fine for smaller UIs, but can become costly with many text elements. If performance becomes an issue, consider caching or precalculating these positions until a property changes (e.g., size, alignment).common/src/main/kotlin/com/lambda/gui/component/core/OutlineRect.kt (2)
35-50: Consider partial updates for outline rendering.Right now,
outlineRectis always called on each render pass, even if properties likeroundRadius,glowRadius, or corner colors remain unchanged. If performance is crucial, caching the outline path or reusing buffers for repeated frames can be beneficial.
54-59: Add usage example forsetColor.
setColor(color)is convenient, but consider adding code comments or examples on how to leverage it in the presence of gradient corners. This ensures future contributors understand its behavior and limitations with multi-colored corners.common/src/main/kotlin/com/lambda/gui/component/layout/Layout.kt (3)
42-42: Consider addressing this TODO comment sooner rather than later.
This TODO suggests an intention to introduce a dedicated subclass or mechanism for alignment. Scheduling its implementation will help ensure complexity doesn’t accumulate and alignment logic remains maintainable.Would you like me to open a new issue or propose a draft implementation to resolve this?
327-335: Clarify destroy contract or allow for safer repeated calls.
Currently, a second call todestroy()throws an exception. In many GUI frameworks, multiple destroy calls simply no-op. Decide if repeated calls should never happen (and thus throwing is appropriate) or if they can happen under valid scenarios. If valid, consider a safer approach.
423-466: Potential overhead from multiple child components.
The companion object factory methods (e.g.,layout,animationTicker,cursorController) can facilitate many child creations. In high-complexity UIs, consider deferring child creation or employing lazy initialization for performance.common/src/main/kotlin/com/lambda/module/modules/client/ClickGui.kt (1)
31-31: Confirm the updated description aligns with user-facing documentation.
The description changed to “sexy again.” Verify that it matches your intended public-facing text and that it remains user-friendly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
common/src/main/kotlin/com/lambda/graphics/animation/Animation.kt(2 hunks)common/src/main/kotlin/com/lambda/graphics/buffer/VertexPipeline.kt(2 hunks)common/src/main/kotlin/com/lambda/graphics/buffer/vertex/ElementBuffer.kt(1 hunks)common/src/main/kotlin/com/lambda/graphics/buffer/vertex/VertexBuffer.kt(1 hunks)common/src/main/kotlin/com/lambda/graphics/pipeline/ScissorAdapter.kt(1 hunks)common/src/main/kotlin/com/lambda/graphics/renderer/gui/font/FontRenderer.kt(1 hunks)common/src/main/kotlin/com/lambda/graphics/renderer/gui/rect/FilledRectRenderer.kt(2 hunks)common/src/main/kotlin/com/lambda/gui/GuiManager.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/core/FilledRect.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/core/OutlineRect.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/core/TextField.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/core/UIBuilder.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/layout/Layout.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/window/AnimatedWindowChild.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/window/TitleBar.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/window/Window.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/window/WindowContent.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleLayout.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleWindow.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/SettingLayout.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/BooleanButton.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/EnumSelector.kt(1 hunks)common/src/main/kotlin/com/lambda/module/hud/TickShiftCharge.kt(2 hunks)common/src/main/kotlin/com/lambda/module/modules/client/ClickGui.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/client/GuiSettings.kt(3 hunks)common/src/main/resources/assets/lambda/shaders/fragment/font/font.frag(1 hunks)common/src/main/resources/assets/lambda/shaders/fragment/signed_distance_field.frag(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- common/src/main/kotlin/com/lambda/graphics/animation/Animation.kt
- common/src/main/kotlin/com/lambda/gui/component/window/TitleBar.kt
- common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/BooleanButton.kt
- common/src/main/kotlin/com/lambda/module/modules/client/GuiSettings.kt
🧰 Additional context used
🧠 Learnings (1)
common/src/main/kotlin/com/lambda/gui/component/core/UIBuilder.kt (1)
Learnt from: Avanatiker
PR: Avanatiker/NeoLambda#98
File: common/src/main/kotlin/com/lambda/gui/component/core/UIBuilder.kt:20-27
Timestamp: 2025-02-21T23:22:59.595Z
Learning: The use of numbers in DSL annotation names (e.g. UIRenderPr0p3rty) is intentional as it affects the DSL color through the name's hash value. This is a design feature, not a naming convention violation.
🪛 detekt (1.23.7)
common/src/main/kotlin/com/lambda/graphics/buffer/VertexPipeline.kt
[warning] 188-190: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (52)
common/src/main/resources/assets/lambda/shaders/fragment/signed_distance_field.frag (2)
15-25: Review performance impact of blur radiusThe current implementation samples 81 texels (9x9 grid) for each output pixel. While this provides good quality for a signed distance field, it could impact performance on lower-end hardware.
Consider making the spread radius configurable via a uniform parameter rather than a hard-coded constant to allow runtime adjustment based on performance requirements.
20-21: Review weight calculation logicThe weight calculation
exp(-color * color)is applied to a vec4 color, which might produce unexpected results as it performs the operation on each color channel independently. Typically, for distance field blurring, weights are based on a scalar distance or alpha value.Consider if this is the intended behavior or if the weight should be calculated from a single component:
- vec4 weight = exp(-color * color); + // Option 1: Use alpha channel if it represents the distance field + float distValue = color.a; + vec4 weight = exp(-distValue * distValue); + + // Option 2: Use color magnitude + // float distValue = length(color.rgb); + // vec4 weight = exp(-distValue * distValue);common/src/main/kotlin/com/lambda/gui/GuiManager.kt (6)
1-16: License block check.
No issues found; the license header appears valid and complete.
18-28: Package and imports look appropriate.
No immediate concerns regarding naming, organization, or unused imports.
29-31: Consider thread safety for mutable data structures.
typeMapis a normalMutableMapthat may pose concurrency risks if accessed by multiple threads. Consider using thread-safe alternatives (e.g., aConcurrentHashMap) or proper synchronization if multi-threaded usage is expected.
32-34: Duplicate registrations remain unhandled.
A previous review comment suggested adding error handling or detection for duplicate type adapters. Reiterating the same suggestion here, as the current implementation silently overrides any existing adapter.
36-46: Initialization of adapters is straightforward.
All registered adapters are neatly organized, and the returned status message provides a useful log for debugging. No additional concerns.
48-57: Null checks for missing adapters.
layoutOfwill returnnullif no matching type adapter is found. This approach is valid, but consider whether you might need logging or an assertion in cases where an adapter is unexpectedly missing.common/src/main/kotlin/com/lambda/gui/component/window/AnimatedWindowChild.kt (1)
42-44: Check for potential null owner references.Accessing
owner?.owner as? Windoworowner?.owner as? AnimatedWindowChildis safe with a cast, but might lead to unexpectednullor type mismatches at runtime ifowner?.ownerdoes not match the assumed type. It might be wise to handle the case where the cast fails or the value is null more gracefully.common/src/main/resources/assets/lambda/shaders/fragment/font/font.frag (1)
1-36: Clean and efficient SDF font shader implementation.This new fragment shader effectively implements a signed distance field (SDF) approach for rendering both fonts and emojis. The implementation handles the two texture types (font and emoji) appropriately with good smoothing transitions via the
smoothstepfunction.A few suggestions to consider:
- Consider adding comments explaining the SDF technique and expected texture formats
- Document the expected range values for
u_SDFMinandu_SDFMaxto guide future developerscommon/src/main/kotlin/com/lambda/graphics/pipeline/ScissorAdapter.kt (3)
18-19: Package relocated appropriately to pipeline namespace.The package relocation from
gltopipelinebetter reflects the adapter role of this component in the rendering pipeline.
28-41: Good refactoring of scissor handling with proper stack management.The scissor mechanism has been well-refactored with improved stack management. The changes simplify the API while maintaining correct nested scissor functionality.
43-58: Improved scissor calculation with proper scaling.The scissor rectangle calculation now correctly uses
RenderMain.scaleFactorfor scaling and properly handles coordinate conversions. The use ofcoerceAtLeast(0.0)prevents negative dimensions which could cause rendering issues.common/src/main/kotlin/com/lambda/graphics/buffer/vertex/VertexBuffer.kt (1)
27-32: Streamlined VertexBuffer class with improved inheritance.The class now extends the abstract
Bufferclass instead of implementingIBufferinterface, which likely reduces code duplication and improves the overall buffer management system.common/src/main/kotlin/com/lambda/gui/component/core/UIBuilder.kt (1)
24-31: Well-structured DSL marker annotations.The DSL marker annotations are appropriately defined to ensure type-safe builders. The unconventional naming in
UIRenderPr0p3rtywith numbers is intentional as it affects the DSL color through the name's hash value, as noted in the retrieved learnings.common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleWindow.kt (1)
38-41: Check multi-tag support
Current filtering usesdefaultTags.firstOrNull() == tag, which may exclude modules having multiple tags. If there's a possibility of multiple applicable tags, consider broadening the filter logic.Would you like me to search the codebase to confirm whether multiple active tags are expected?
common/src/main/kotlin/com/lambda/gui/impl/clickgui/SettingLayout.kt (1)
78-80: Monitor text shrinking
scale *= 0.92can repeatedly shrink the text over time. Check whether this dynamic resizing is intentional, or switch to a static scale for consistent presentation.common/src/main/kotlin/com/lambda/module/hud/TickShiftCharge.kt (3)
21-24: Use of new rendering imports is consistent
These imports provide direct access to the new rendering functions, simplifying calls tofilledRectandoutlineRect.
42-46: Progress animation logic looks correct
Ensuring the value is clamped in the range [0..1] avoids invalid progress values, aligning well with UI progress bar expectations.
53-55: Switched corner radius toClickGui.roundRadius
Replacing any previous radius reference withClickGui.roundRadiusis coherent with the updated corner style.common/src/main/kotlin/com/lambda/gui/component/window/WindowContent.kt (5)
1-16: License header check
The GPL license text is standard; no issues noted.
18-28: Imports look consistent
Imports for animation, layout, and math match the file’s usage.
29-66: Vertical layout withlistify()
UsingoverrideYto stack children vertically is straightforward. Verify positioning logic if a child’s height changes at runtime to ensure no unwanted overlap.
119-127: Dynamic layout height
Combining child animations (showAnimation/staticShowAnimation) is sensible. Confirm child components always return valid animation values to avoid unexpected layout jumps.
129-136: ConvenientwindowContentbuilder
Provides a quick way to attach scrollable content to a window. No issues found.common/src/main/kotlin/com/lambda/gui/component/window/Window.kt (7)
1-16: License header check
The GPL license text is standard; no issues found.
18-34: Imported references for UI building
All utility and GUI-related imports appear accurate and necessary.
85-88: Expand/minimize action lists
Deferring window-specific behavior to these callbacks is flexible. Ensure closures remain valid if the window is hidden or the layout reinitialized.
109-127: Minimize toggle & dimension tracking
LinkingisMinimizedto the actual window height ensures clarity. External references to width/height should respect these animated transitions.
128-158: Animated dimension properties
UsingwidthAnimationandheightAnimationfor smooth transitions is well-structured. Remember to re-check if dynamic content changes are always reflected whenautoResizeis active.
239-246: Expose or configure resizing constraints
Hard-codedcoerceIn(80.0, 1000.0)can be restrictive for some display setups.
292-311: Window builder pattern
Convenient creation of aWindowwith customizable content. No issues discovered.common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleLayout.kt (1)
125-127: Validate layout constraints for dynamic size changes.You override the X coordinate and width to fit within the owner layout. Ensure that the resulting size does not lead to overlapping or negative widths if the owner’s size becomes smaller than the specified padding. Adding boundary checks or a minimum width can prevent rendering anomalies.
common/src/main/kotlin/com/lambda/gui/component/core/TextField.kt (1)
32-44: Ensure text properties match various font renderers.The computed
textWidthandtextHeightrely onFontRenderer, which may differ if custom or system fonts are used. Consider adding a fallback or a defensive check to ensure the returned dimensions are valid, particularly when switching to alternative font renderers or font sizes.common/src/main/kotlin/com/lambda/gui/component/core/FilledRect.kt (5)
25-37: Well-structured property definitions.The usage of
@UIRenderPr0p3rtyfor both corner radii and color properties is consistent and avoids clutter. The defaults look sensible, and the code is clear.
40-57: Ensure consistent radius handling.The
onRenderblock correctly gathers all corner radii and color values to render a filled rectangle. This approach centralizes the rendering logic in thefilledRectcall. Just confirm that any outside changes to these properties invalidate the rendering cache if the framework caches layout results.
59-64: Radius assignment logic is readable and intuitive.Both
setRadius(Double)andsetRadius(Double, Double, Double, Double)help set top and bottom corners in a straightforward manner. This ensures flexibility for the layout developer.Also applies to: 66-76
78-83: Use of color gradient helpers is commendable.Providing
setColor,setColorH, andsetColorVcovers common gradient scenarios, making the API more versatile. Good job on ensuring corner color assignments remain consistent.Also applies to: 85-97
99-125: Extension functions for layout integration are well-organized.The approach of
rect,rectBehind, andrectOveris readable, enabling developers to insert layer-based rectangles easily. This follows a clean builder pattern.common/src/main/kotlin/com/lambda/graphics/renderer/gui/rect/FilledRectRenderer.kt (3)
30-31: Clarify alpha threshold logic.Currently,
MIN_ALPHAis set to 1. In AWT, color alpha ranges from 0 to 255. Checkingif (alpha < 1)might effectively skip only fully transparent colors, but it could also skip semitransparent colors if fractional alpha values are interpreted differently. Consider clarifying or adjusting this threshold if partial transparency is intended.
81-93: Skip rendering only if fully transparent.Skipping rendering when all corners have alpha below
MIN_ALPHAis a reasonable optimization. Just ensure there's no scenario where partial transparency is erroneously dropped.
113-118: Efficient usage of 3D vectors for 2D quads.Defining quad corners with
vec3m(...).vec2(...)is consistent with typical buffers. Confirm that any Z-based transformations in the pipeline handle these coordinates correctly if layering or depth is introduced in the future.common/src/main/kotlin/com/lambda/graphics/renderer/gui/font/FontRenderer.kt (6)
36-40: Good separation of configuration getters.Using dedicated properties (
shadowShift,baselineOffset,gap) ensures customization remains centralized. This makes maintenance easier and avoids magic constants in the rendering code.Also applies to: 41-47
58-76: Flexible text rendering method.
drawStringproperly sets up the shader, binds textures, and processes text. The injection of shadow logic and emoji parsing withinprocessTextis a neat separation of concerns, preserving clarity.
86-107: Dedicated glyph rendering function is helpful.
drawGlyphenables modular rendering, an advantage when dealing with custom glyph-based components or partial text rendering. This fosters code reuse and composability.
118-138: Well-structuredbuildGlyphimplementation.Using an extension on
VertexPipelinefor glyph construction keeps low-level buffer operations encapsulated. It's a clean approach that promotes maintainability.
156-162: Check for potential undercounting of width with newlines.The logic increments
widthbychar.widthplus gaps. Ensure that multiple newlines do not create leftover vertical spacing that might need a separate measure. Currently, newlines resetposXbut not the measuring logic, which might be acceptable for single-line usage.
185-213: Potential complexity in nested emoji parsing.
processTextSectionrecursively handles text blocks around emojis. While functional, consider carefully testing nested cases (e.g., multiple emojis in the same line, or edge cases with consecutive emojis) to ensure correct slicing and position increments.Also applies to: 215-252
common/src/main/kotlin/com/lambda/gui/component/layout/Layout.kt (2)
49-53: Validate clamping logic when child size exceeds the parent.
WhenclampPositionis enabled, large children relative to the parent’s width or height can force positions to clamp to 0, potentially overlaying them awkwardly. Ensure that the clamping behavior gracefully handles situations where a child is larger than its parent.Also applies to: 54-59
394-394: Avoid forcibly releasing mouse clicks on non-hovered layouts.
Line 394 sets the action toMouse.Action.Releaseif the layout is not hovered. This might break scenarios where a click or drag event started on one layout and ended on another. Evaluate if there’s a more nuanced decision process.common/src/main/kotlin/com/lambda/module/modules/client/ClickGui.kt (2)
67-67: Avoid usingowner!!to prevent potential runtime crashes.
Relying on!!can cause unexpectedNullPointerExceptionif the UI hierarchy changes. Consider a safe call (owner?.rect) and handle null gracefully, or ensure the owning context is always non-null.
80-83: Evaluate the self-toggle flow on enable.
Immediately toggling the module off after showing the screen can be surprising if a user expects to manually disable it. If auto-disabling is intended, clarify why. Otherwise, let the user close the GUI or hide it themselves.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/graphics/texture/TextureUtils.kt(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (3)
common/src/main/kotlin/com/lambda/graphics/texture/TextureUtils.kt (3)
29-29: Good work extracting the compression level to a constant.Extracting the compression level value to a named constant improves code readability and maintainability.
28-73:Details
❓ Verification inconclusive
Verify that removed functionality has been migrated to the new texture API.
According to the PR description, significant functionality has been removed from this file including:
- The
metricCachevariable for font metrics caching- The
uploadmethod for BufferedImage- The
setupLODmethod for texture parameters- The
getCharImagemethod for character image generation- The
rescaleextension functionEnsure that equivalent functionality exists in the new texture API mentioned in the PR objectives, particularly in the new
TextureOwnerclass.
🏁 Script executed:
#!/bin/bash # Look for the new TextureOwner class and its methods rg -t kotlin "class TextureOwner" -A 20 # Check for new methods that might replace the old upload functionality rg -t kotlin "fun upload" --glob "**/*Texture*.kt" # Check for methods related to LOD setup in the new API rg -t kotlin "setupLOD|texParameter" --glob "**/*Texture*.kt"Length of output: 693
Action Required: Confirm Migration of All Texture Functionality
Based on our verification:
- The new API in common/src/main/kotlin/com/lambda/graphics/texture/Texture.kt now includes:
- The
uploadmethod (with both BufferedImage and ByteBuffer variants)- The
setupLODmethod (used within texture parameter setup)- However, we did not locate any evidence of the new
TextureOwnerclass—nor did we find migrated equivalents for:
- The
metricCachevariable for font metrics caching- The
getCharImagemethod for character image generation- The
rescaleextension functionPlease confirm that these missing functionalities have been intentionally removed or migrated elsewhere as per the PR objectives.
71-72:Details
✅ Verification successful
Breaking change: Default parameter for format has been removed.
The second overload of
readImagealso had its default parameter removed. All calling code needs to be updated to explicitly specify the format.
🏁 Script executed:
#!/bin/bash # Check if there are any calls to the second overload of readImage that might need updating rg -A 3 "readImage\([^,]+\)" --type kotlinLength of output: 175
Action Required: Ensure explicit format specification where applicable
I verified that, aside from the unrelated implementation in
LambdaResource.readImage(), there are no remaining call sites that rely on the previously provided default parameter forformatin thereadImageoverload. The breaking change incommon/src/main/kotlin/com/lambda/graphics/texture/TextureUtils.kt(lines 71–72) appears isolated. Please confirm that any future invocations of the TextureUtilsreadImagemethods explicitly pass a validNativeImage.Formatargument.
| format: NativeImage.Format, | ||
| ): Long { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking change: Default parameter for format has been removed.
The removal of the default parameter for format means that all callers of this method will now need to explicitly provide a format. Ensure all usages of this method throughout the codebase have been updated.
🏁 Script executed:
#!/bin/bash
# Look for calls to readImage that might be affected by the removed default parameter
rg -A 2 "readImage\(" --type kotlinLength of output: 2145
Breaking Change: Mandatory Explicit Format Parameter
The default value for the format parameter has now been removed from the readImage method in TextureUtils.kt (lines 55-56). This change requires that every call site supplies an explicit NativeImage.Format value. Notably, the call in common/src/main/kotlin/com/lambda/graphics/texture/TextureOwner.kt (i.e. Texture(path.readImage(), levels = mipmaps)) still omits the format parameter and will need to be updated accordingly. Please review all usages of this method to ensure they are passing an explicit format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
common/src/main/kotlin/com/lambda/gui/GuiManager.kt (2)
38-52: Optional: Provide a mechanism for additional adapters.The
load()method hard-codes adaptations forBooleanSetting,EnumSetting, andFunctionSetting, but the PR objectives mention more advanced GUI configuration possibilities. You might consider adding custom adapters here or supporting runtime discovery to handle future UI element types seamlessly.
54-63: Optional: Handle unregistered references.
layoutOfreturnsnullif no matching adapter is found intypeMap. Consider adding logging or a fallback to provide better insight when no adapter is found, especially in large-scale or production GUI systems.common/src/main/kotlin/com/lambda/config/settings/FunctionSetting.kt (1)
1-16: Update the copyright year to match current year.The copyright notice states the year as 2025, but the current year is 2024.
-/* - * Copyright 2025 Lambda +/* + * Copyright 2024 Lambdacommon/src/main/kotlin/com/lambda/config/Configurable.kt (2)
424-429: Consider supporting generic return types in the function setting method.While the
FunctionSettingclass supports functions that return any non-nullable type, this method is limited to functions that returnUnit. Consider creating an overload that supports generic return types.+/** + * Creates a [FunctionSetting] with the provided parameters and adds it to the [settings]. + * + * @param name The unique identifier for the setting. + * @param defaultValue The function to be stored as the setting's value. + * @param description A brief explanation of the setting's purpose and behavior. + * @param visibility A lambda expression that determines the visibility status of the setting. + * + * @return The created [FunctionSetting]. + */ fun setting( name: String, defaultValue: () -> Unit, description: String = "", visibility: () -> Boolean = { true } ) = FunctionSetting(name, defaultValue, description, visibility).register() +/** + * Creates a [FunctionSetting] with the provided parameters and adds it to the [settings]. + * + * @param name The unique identifier for the setting. + * @param defaultValue The function to be stored as the setting's value. + * @param description A brief explanation of the setting's purpose and behavior. + * @param visibility A lambda expression that determines the visibility status of the setting. + * + * @return The created [FunctionSetting]. + */ +inline fun <reified T : Any> setting( + name: String, + defaultValue: () -> T, + description: String = "", + noinline visibility: () -> Boolean = { true } +) = FunctionSetting(name, defaultValue, description, visibility).register()
424-429: Add documentation comment for the new setting method.The other setting methods have detailed documentation comments. Adding similar documentation to this new method would maintain consistency.
+/** + * Creates a [FunctionSetting] with the provided parameters and adds it to the [settings]. + * + * @param name The unique identifier for the setting. + * @param defaultValue The function to be executed when the setting is triggered. + * @param description A brief explanation of the setting's purpose and behavior. + * @param visibility A lambda expression that determines the visibility status of the setting. + * + * @return The created [FunctionSetting]. + */ fun setting( name: String, defaultValue: () -> Unit, description: String = "", visibility: () -> Boolean = { true } ) = FunctionSetting(name, defaultValue, description, visibility).register()common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/UnitButton.kt (3)
2-2: Copyright year should be updatedThe copyright year is set to 2025, which is in the future. Consider updating it to the current year (2024) or the year when the code was actually written.
- * Copyright 2025 Lambda + * Copyright 2024 Lambda
31-33: Consider adding error handlingThe click handler directly calls
setting.value()without any error handling. If this function throws an exception, it might cause issues in the UI.onMouseClick(Mouse.Button.Left, Mouse.Action.Click) { - setting.value() + try { + setting.value() + } catch (e: Exception) { + // Handle or log the exception + logger.error("Error executing setting function", e) + } }
37-39: Enhance documentation for clarityWhile the documentation comment is concise, it could benefit from more details about parameters, usage examples, or return values to help other developers understand how to use this component.
/** * Creates a [UnitButton] - visual representation of the [FunctionSetting] +* +* @param setting The function setting to be represented by this button +* @return The created UnitButton instance that has been added to this layout +* +* Example usage: +* ``` +* layout.unitSetting(myFunctionSetting) +* ``` */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
common/src/main/kotlin/com/lambda/config/AbstractSetting.kt(1 hunks)common/src/main/kotlin/com/lambda/config/Configurable.kt(2 hunks)common/src/main/kotlin/com/lambda/config/settings/FunctionSetting.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/GuiManager.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/UnitButton.kt(1 hunks)common/src/main/kotlin/com/lambda/module/Module.kt(3 hunks)common/src/main/kotlin/com/lambda/module/modules/debug/SettingTest.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/kotlin/com/lambda/module/Module.kt
🔇 Additional comments (9)
common/src/main/kotlin/com/lambda/config/AbstractSetting.kt (1)
129-131: Access to reset functionality is now public.The
reset()method visibility has been changed from private to public, allowing external classes to reset settings to their default values. This is a useful enhancement that enables more flexible management of settings throughout the application.common/src/main/kotlin/com/lambda/gui/GuiManager.kt (1)
34-36: Consider handling duplicate adapter registrations ().A past review mentioned adding safeguards to prevent duplicate registrations. Currently,
typeAdapteroverwrites existing mappings when the same type is registered again. This can lead to unexpected behavior, particularly if multiple modules attempt to register the same type.common/src/main/kotlin/com/lambda/module/modules/debug/SettingTest.kt (2)
22-22: Added required import for Communication utility.The import for
Communication.infois correctly added to support the new unit setting that logs information when activated.
65-67: Good addition of action-based setting demonstrating the new GUI capabilities.This new
unitSettingdemonstrates a different type of setting that executes an action (logging a message) rather than storing a value. This is a good test case that aligns with the new GUI API capabilities mentioned in the PR description.The implementation correctly uses a lambda function with proper scoping (
this@SettingTest) to ensure the action is executed in the correct context.common/src/main/kotlin/com/lambda/config/settings/FunctionSetting.kt (1)
25-38: Well-designed implementation for function settings.The
FunctionSettingclass provides a clean way to store function references as settings. The implementation correctly:
- Uses generics to support functions returning any non-nullable type
- Doesn't attempt to serialize the function (returning JsonNull)
- Resets to default value when loading from JSON
common/src/main/kotlin/com/lambda/config/Configurable.kt (2)
26-26: Import for new FunctionSetting correctly added.The import is properly added to support the new setting type.
33-33: Complex settings imports consolidated.Changing to a wildcard import simplifies the import section.
common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/UnitButton.kt (2)
26-34: Good implementation of the button componentThe
UnitButtonclass is well-structured and follows a clean object-oriented design. It properly extendsSettingLayoutwith appropriate type parameters and implements the necessary functionality for executing an action when clicked.
37-42: Good use of extension function with builder patternThe companion object effectively provides a clean, builder-style API for creating
UnitButtoninstances. The use of@UIBuilderannotation and extension function onLayoutimproves readability and integration with the UI system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (21)
common/src/main/kotlin/com/lambda/gui/impl/clickgui/core/AnimatedChild.kt (1)
34-134: Address the TODO and ensure UI stability.Overall, the animations, visibility checks, and layout destruction are well-structured. However, note the “ToDo: replace with timer” comment on line 51. A dedicated timer or utility might provide more robust handling than manual time stamping. Also consider any edge cases involving:
- Minimal or zero-sized windows
- Rapidly toggling visibility or minimization states
Feel free to ask if you'd like a sample refactor for the timed hover logic.
common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/BooleanButton.kt (1)
1-16: The copyright year should be 2024 instead of 2025.The copyright notice shows 2025, but according to the PR information, this should be 2024 since we're currently in March 2025.
-/* - * Copyright 2025 Lambda +/* + * Copyright 2024 Lambdacommon/src/main/kotlin/com/lambda/gui/GuiManager.kt (1)
60-70: Consider adding null safety to the layout lookup.The
layoutOffunction could benefit from more robust null safety when handling inheritance-based lookups.inline fun Layout.layoutOf( reference: Any, block: Layout.() -> Unit = {} ): Layout? = - (typeMap[reference::class] ?: typeMap.entries.firstOrNull { - reference::class.java.superclass == it.key.java - }?.value)?.invoke(this, reference)?.apply(block) + (typeMap[reference::class] + ?: typeMap.entries.firstOrNull { entry -> + reference::class.java.superclass?.let { superclass -> + superclass == entry.key.java + } ?: false + }?.value)?.invoke(this, reference)?.apply(block)common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleWindow.kt (2)
32-33: Remove TODO comment about the tag system.The code contains a TODO comment about the tag system, but since this PR represents "five months of development work," this comment should either be addressed or removed before merging.
class ModuleWindow( owner: Layout, - val tag: ModuleTag, // todo: tag system + val tag: ModuleTag, initialPosition: Vec2d ) : Window(owner, tag.name, initialPosition, minimizing = Minimizing.Absolute, autoResize = AutoResize.ByConfig) {
44-52: Consider using state consistency instead of event-based minimization.The code sets up handlers to minimize modules when the window expands or minimizes, but this approach could lead to state inconsistency if the window state changes through other means.
- val minimize = { - modules.forEach { - it.isMinimized = true - } - } - - onWindowExpand { minimize() } - onWindowMinimize { minimize() } + // Maintain state consistency through a property observer + var windowState: WindowState = WindowState.Normal + set(value) { + field = value + if (value == WindowState.Expanded || value == WindowState.Minimized) { + modules.forEach { it.isMinimized = true } + } + } + + onWindowExpand { windowState = WindowState.Expanded } + onWindowMinimize { windowState = WindowState.Minimized }common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt (5)
1-16: The copyright year should be 2024 instead of 2025.The copyright notice shows 2025, but according to the PR information, this should be 2024.
-/* - * Copyright 2025 Lambda +/* + * Copyright 2024 Lambda
34-36: Add space after the generic type parameter.There should be a space after the generic type parameter in the class declaration.
-abstract class SettingSlider <V : Any, T: AbstractSetting<V>>( +abstract class SettingSlider<V : Any, T : AbstractSetting<V>>(
37-39: Define animation property purpose in code comments.The
changeAnimationproperty lacks documentation explaining its purpose and usage within the slider.+ /** + * Animation that controls the visual feedback when the setting value changes + */ private var changeAnimation by animation.exp(0.0, 1.0, 0.5) { true }
64-82: Improve variable naming and consider extracting animation logic.The variable
lastValuecould be named more descriptively, and the animation logic could be extracted to improve readability.textField { - var lastValue = "" + var previousTextValue = "" + + /** + * Triggers the change animation if the text has changed + */ + fun updateChangeAnimationIfNeeded(newText: String) { + if (previousTextValue != newText) { + changeAnimation = 0.0 + previousTextValue = newText + } + } onUpdate { - lastValue = text text = settingDelegate.toString() - if (lastValue != text) changeAnimation = 0.0 + updateChangeAnimationIfNeeded(text)
30-33: Unused imports in the SettingSlider class.Several math imports (PI, pow, sin) are included but not used in the code.
import com.lambda.gui.impl.clickgui.module.SettingLayout import com.lambda.module.modules.client.ClickGui import com.lambda.util.math.lerp -import kotlin.math.PI -import kotlin.math.pow -import kotlin.math.sincommon/src/main/kotlin/com/lambda/gui/component/window/WindowContent.kt (2)
54-67: Consider handling out-of-order or dynamically inserted children.
listify()arranges children only on creation or reflow, but dynamic inserts/removals at runtime may disrupt ordering. Consider adding checks or triggers that calllistify()again once new children are inserted or removed.
110-114: Make scrolling speed configurable.Using a fixed multiplier (
dwheel += delta * 10.0) might feel too fast or too slow for some users. Exposing a setting for scroll sensitivity could enhance usability.common/src/main/kotlin/com/lambda/gui/impl/clickgui/core/SliderLayout.kt (1)
38-39: Consider more robust approach to managingSettingSlider.
The code castsownertoSettingSlider<*,*>, but if that cast fails, the reference isnull, which may lead to unexpected behavior ifowneris not aSettingSlider. Consider requiring aSettingSliderin the constructor or provide a fallback strategy to handle non-SettingSliderowners.common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/ModuleLayout.kt (3)
52-85: Extract complex animation logic for better maintainability.
This block combines multiple reversed animations for enabling, hovering, and opening. Consider refactoring into a separate helper method or class for clarity, testability, and troubleshooting.
139-151: Allow multiple modules to remain open if desired.
When one module expands, this logic closes all others unlessClickGui.multipleSettingWindowsistrue. Confirm that a single expanded module is the ideal UX or consider offering a user setting to open multiple modules concurrently.
174-176: Prefer a more descriptive method name or add explicit KDoc.
moduleLayout(module: Module)is terse. Consider a name likeaddModuleLayoutfor clarity, or add more KDoc to clarify its purpose and behavior for future maintainers.common/src/main/kotlin/com/lambda/gui/component/core/OutlineRect.kt (2)
27-29: Clarify howroundRadiusandglowRadiusinteract.
Both properties affect rendering differently, but the exact relationship is unclear. Documenting their intended ranges (e.g., 0 to ∞) or providing usage notes will help other developers configure them confidently.
54-73: Reduce duplication among color setting methods.
setColor,setColorH, andsetColorVeach assign corner colors. Consider consolidating shared logic in a helper method to adhere to DRY principles and reduce future maintenance overhead.common/src/main/kotlin/com/lambda/gui/component/layout/Layout.kt (3)
42-43: Address the TODO for advanced alignment logic.
The inline// ToDo: impl alignmentLayout: Layout, ...could be a blocker if you plan to support more elaborate alignment use cases soon. Consider scheduling or implementing it sooner to avoid confusion.
49-62: Handle child size exceeding owner’s boundaries.
When the child’s width or height is larger than the owner’s, clamping the position to[0, ownerSize - childSize]can lead to unexpected layout overlaps or forced zero positioning. Consider providing a fallback (e.g., partial visibility or scroll support) to improve usability in such edge cases.
77-79: Prevent problematic negative sizes.
Negative or zero sizes may cause unexpected clipping, alignment, or scissoring behaviors. If not supported, consider adding a safeguard:@UIRenderPr0p3rty var width = 0.0 + set(value) { field = value.coerceAtLeast(0.0) } @UIRenderPr0p3rty var height = 0.0 + set(value) { field = value.coerceAtLeast(0.0) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
common/src/main/kotlin/com/lambda/config/settings/FunctionSetting.kt(1 hunks)common/src/main/kotlin/com/lambda/graphics/buffer/VertexPipeline.kt(1 hunks)common/src/main/kotlin/com/lambda/graphics/renderer/gui/rect/FilledRectRenderer.kt(2 hunks)common/src/main/kotlin/com/lambda/graphics/renderer/gui/rect/OutlineRectRenderer.kt(4 hunks)common/src/main/kotlin/com/lambda/gui/GuiManager.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/core/OutlineRect.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/layout/Layout.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/window/TitleBar.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/window/Window.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/component/window/WindowContent.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleWindow.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/core/AnimatedChild.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/core/SliderLayout.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/ModuleLayout.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/SettingLayout.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/BooleanButton.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/EnumSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/NumberSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/UnitButton.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/client/ClickGui.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- common/src/main/kotlin/com/lambda/graphics/buffer/VertexPipeline.kt
- common/src/main/kotlin/com/lambda/gui/impl/clickgui/settings/UnitButton.kt
- common/src/main/kotlin/com/lambda/config/settings/FunctionSetting.kt
- common/src/main/kotlin/com/lambda/gui/component/window/TitleBar.kt
- common/src/main/kotlin/com/lambda/graphics/renderer/gui/rect/FilledRectRenderer.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (37)
common/src/main/kotlin/com/lambda/graphics/renderer/gui/rect/OutlineRectRenderer.kt (7)
37-39: Good architectural change to singleton patternChanging from a class to a singleton object makes sense for renderers that don't need instance-specific state. This follows the principle that stateless utility components should be singletons. The inheritance change to
AbstractGUIRendereralso suggests a cleaner renderer hierarchy.
22-25: Appropriate import updatesThe new imports reflect the restructured architecture with the new GUI rendering system. Using the shader companion's
shader()function appears to be part of a more consistent shader management approach.
40-41: Better constant declaration withconst valGood improvement changing from mutable properties to
const val. This makes the constants truly immutable and allows for compile-time optimization, which can improve performance.
43-49: Improved method naming frombuildtooutlineRectThe rename from
buildtooutlineRectis more descriptive and clearly indicates the method's purpose. This enhances code readability and follows better naming practices.
60-63: Enhanced rendering control flowThe change from
pipeline.usetorender(shade)improves the control flow with a more structured rendering approach. The early return inside the lambda is clean and prevents unnecessary work for very small glow radii.
73-85: Improved UV mapping in vertex generationThe addition of UV mapping calculations using the
transformfunction provides better texture coordinate generation. The vertex creation is now more streamlined with the builder pattern approach.
104-108: Consistent use of constantsGood change replacing the variable reference with the
VERTICES_COUNTconstant directly, which makes the code more consistent and easier to maintain.common/src/main/kotlin/com/lambda/module/modules/client/ClickGui.kt (4)
34-64: New settings look good.These newly added settings (titleBarHeight, moduleHeight, etc.) provide helpful customization for the GUI layout and styling. They’re straightforward, and the range constraints look reasonable.
69-69: Avoid using the not-null assertion operator.Relying on
owner!!can lead to runtime crashes ifowneris null. Consider using safe calls or adding null checks to ensure a more robust implementation.
74-78: Replace hard-coded spacing and add window bounds checks.
- The constants
var x = 10.0and the+ 5increments are hard-coded, which limits customization. Consider extracting them into configurable properties.- There's no check to keep modules within screen bounds (e.g., reset X and increase Y upon overflow). This can result in off-screen windows.
88-91: Confirm the toggle logic.Calling
toggle()immediately afterSCREEN.show()may disable the module right away, possibly preventing users from seeing or interacting with the GUI. Verify if this behavior is intentional.common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/NumberSlider.kt (1)
28-58: Consider edge cases and add tests.The logic for converting, interpolating, and rounding numeric settings works well. However, consider verifying behavior for:
- Reversed or invalid ranges (e.g.,
min > max)- Zero or negative steps
- Large numeric values that might lead to floating-point precision issues
Also, add a few unit tests confirming these corner cases, ensuring that the slider behaves predictably under all circumstances.
common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/EnumSlider.kt (1)
26-56: Double-check boundary conditions and test coverage.Your approach for mapping the slider’s position to an enum’s ordinal is conceptually sound. However, consider verifying:
- Behavior if the Enum has only one value or an unexpected ordinal range
- Potential “skipping” of intermediate enum values if the user drags the slider quickly
Adding dedicated unit tests covering these boundary scenarios will help catch any off-by-one errors or unexpected user interactions.
common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/BooleanButton.kt (5)
35-39: LGTM! Clean implementation of the BooleanButton component.The class structure clearly extends SettingLayout with proper generic type parameters for Boolean and BooleanSetting.
42-63: Well-structured checkbox implementation with proper animations.The checkbox implementation makes good use of reactive updates with the onUpdate handler, sets appropriate cursor behavior, and handles animations smoothly.
65-82: Nicely implemented knob animations with smooth transitions.The knob component provides fluid animations between states using lerp functions for both position and color, ensuring a polished user experience.
93-95: Simple and effective click handler.The click handler toggles the setting value directly, which is appropriate for this component.
99-104: Clean extension function for declarative UI building.The companion object provides a well-documented extension function that follows the builder pattern, making it easy to create boolean settings in layouts.
common/src/main/kotlin/com/lambda/gui/GuiManager.kt (2)
36-38: Add error handling for duplicate type adapter registration.The typeAdapter function should check for duplicate registrations to prevent overwriting existing adapters without warning.
private inline fun <reified T : Any> typeAdapter(noinline block: (Layout, T) -> Layout) { + if (typeMap.containsKey(T::class)) { + throw IllegalStateException("Type adapter for ${T::class.simpleName} is already registered") + } typeMap[T::class] = { owner, converted -> block(owner, converted as T) } }
40-58: LGTM! Clear implementation of type adapters for settings.The
load()method cleanly registers type adapters for different setting types. The implementation is straightforward and follows a consistent pattern.common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleWindow.kt (2)
59-65: LGTM! Clean implementation of the module window builder.The companion object extension function follows the builder pattern well, making it easy to create and configure module windows.
38-42:Details
✅ Verification successful
Verify filter approach for modules.
The code filters modules based on the first tag only, which may not be the intended behavior if modules can have multiple relevant tags.
🏁 Script executed:
#!/bin/bash # Check if modules can have multiple tags and whether filtering by first tag only is appropriate # Look for module implementations with multiple tags rg -A 3 "defaultTags\s*=\s*listOf\(" --glob "*.kt" # Check other locations that may handle module tags rg "firstOrNull\(\)\s*==\s*tag" --glob "*.kt"Length of output: 228
Module Filtering Approach Verified – No Changes Needed
I analyzed the filtering logic using our search tool and confirmed that the only instance using tag filtering is in
common/src/main/kotlin/com/lambda/gui/impl/clickgui/ModuleWindow.kt, where modules are filtered by the first default tag. There’s no evidence in the codebase of alternative filtering for multiple tags, so the current approach appears intentional.common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/SettingLayout.kt (2)
80-83: Be cautious with continuously scaling down the text.Within
titleBar.textField.onUpdate { scale *= 0.92 }, each update frame repeatedly shrinks the text. This may cause it to become overly small or even disappear over time. Verify if this behavior is intentional or if you meant to apply a one-time scale factor.Would you like me to generate a quick script to search for all references to
titleBar.textField.onUpdatein the repo to confirm if there's a single-time or repeated scale pattern?
61-66: Confirm content destruction logic for non-expandable settings.Calling
content.destroy()whenexpandable == falsepermanently removes any child components that might be important for layout continuity or future expansions. Verify if this is the intended behavior or if you need a mere hide/disabling mechanism.common/src/main/kotlin/com/lambda/gui/component/window/Window.kt (2)
236-241: Expose or configure size constraints.Hard-coded size bounds
(coerceIn(80.0, 1000.0))can be limiting for different screen resolutions and user preferences. Consider making them dynamic or configurable via a setting.
80-84: Validate expand/minimize side effects.When toggling
isMinimized, the code iterates over a list of actions inexpandActionsorminimizeActions. Confirm that no concurrency or ordering concerns arise if these actions modify the same underlying GUI states.common/src/main/kotlin/com/lambda/gui/impl/clickgui/core/SliderLayout.kt (3)
75-79: Guard against zero or negative width in slider transform.
When computingdragProgress,transform(...)usesbg.widthin the range calculation. Ifbg.widthbecomes zero, this may introduce unexpected division by zero scenarios. Consider either ensuringbg.widthremains positive or adding a check before transformation.Would you like me to search for references to
bg.widthin your codebase to verify whether it can be zero?
92-101: Confirm layering of progress and outline.
You're rendering both a progress rectangle and an outline referencing the samerect. Ensure this layering is intentional and the outline is always drawn on top. If it can be visually obscured, consider usingrectOver(...)or adjusting render order.
35-139: Overall slider design looks solid.
Beyond the minor casting and edge-case concerns, the slider logic is well-organized, with clear property blocks for progress and animations. Good job!common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/ModuleLayout.kt (2)
123-126: Confirm that left-click toggling matches user expectations.
Toggling a module via a single left-click might conflict with a typical drag-to-move pattern or double-click approach. Verify this is intentional and won't cause confusion for users.
36-218: Great modular approach overall.
The module’s layout, animation, and event handling are well-organized and extensible. With the smaller refinements suggested, this is shaping up nicely.common/src/main/kotlin/com/lambda/gui/component/core/OutlineRect.kt (1)
75-101: Builder methods are straightforward and cohesive.
Theoutline,outlineBehind, andoutlineOverfunctions cleanly integrate with theUIBuilderpattern. This consistency is excellent for reusability.common/src/main/kotlin/com/lambda/gui/component/layout/Layout.kt (5)
1-16: License header looks consistent.
No immediate issues with the GPL header.
121-121: Revisit minimal rendering dimensions.
renderSelfis disabled if width or height is ≤ 1. This might hide elements intended to be 1-pixel thick (e.g., lines or separators). Double-check whether this threshold is too restrictive.
282-290: Confirm whether child layouts need manual cleanup.
Currently,destroy()only removes this layout from its owner’s children, leaving the removal (or potential recursion) of its own children unaddressed. Verify if child resources or references need explicit disposal.
304-383: Event propagation logic might skip rendering for children.
Returning early on certain events (e.g.,Render) means children only get rendered ifrenderSelfis true for the parent. Ensure this matches your design for layered or hidden-yet-interactive components.
385-455: Utility methods are well-structured.
The builder pattern for creating layouts, animation tickers, and cursor controllers is clear and aligns well with a modular DSL-like approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/NumberSlider.kt (1)
34-48: Slider initialization logic looks robust.The initialization block correctly:
- Sets up the slider's progress using a transformation function that maps the current setting value from min-max to 0.0-1.0 range
- Defines an
onSlideevent handler that updates the setting value with appropriate interpolation and step roundingA minor suggestion would be to add validation for the initial setting value to ensure it falls within the defined range.
init { + // Ensure initial value is within bounds + if (settingDelegate.toDouble() < min || settingDelegate.toDouble() > max) { + settingDelegate = settingDelegate.typeConvert(settingDelegate.toDouble().coerceIn(min, max)) + } + slider.progress { transform( settingDelegate.toDouble(), min, max, 0.0, 1.0 ) } // Rest of the code remains the same }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
common/src/main/kotlin/com/lambda/graphics/animation/Animation.kt(2 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/NumberSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/client/GuiSettings.kt(3 hunks)common/src/main/kotlin/com/lambda/util/math/MathUtils.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- common/src/main/kotlin/com/lambda/graphics/animation/Animation.kt
- common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt
- common/src/main/kotlin/com/lambda/module/modules/client/GuiSettings.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (5)
common/src/main/kotlin/com/lambda/util/math/MathUtils.kt (1)
42-52: Improved numerical precision with BigDecimal.The rewrite of
roundToStepusing BigDecimal addresses potential floating-point precision issues in the previous implementation. This is a good enhancement, especially for financial calculations or UI components where precise decimal rounding is critical.The implementation correctly:
- Handles division by zero
- Uses appropriate rounding mode (HALF_UP)
- Preserves original scale of the step value
- Properly converts back to the original number type
common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/NumberSlider.kt (4)
1-16: License header looks good.The GNU GPL v3 license header is properly included with correct formatting and copyright information.
18-27: Appropriate imports and package declaration.The package declaration and imports are well-organized. The code correctly imports required components including the necessary math utility functions for slider operation.
28-33: Well-defined class signature with proper generic constraints.The class definition is well-structured with appropriate generic constraints. The
NumberSliderclass properly extendsSettingSliderand requires that the generic typeVis both aNumberandComparable<V>, which are suitable constraints for a numeric value that needs to be compared for range validation.The properties
minandmaxare correctly initialized from the setting's range by converting to Double for consistent handling.
50-58: Companion object provides a clean DSL builder approach.The companion object follows best practices by:
- Using the
@UIBuilderannotation for DSL support- Providing clear documentation for the extension function
- Using a concise builder pattern with
apply(children::add)to integrate with the layout systemThis approach ensures a clean, fluent API for creating numeric sliders in layouts.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
common/src/main/kotlin/com/lambda/graphics/renderer/gui/font/FontRenderer.kt (6)
41-42: Consider caching repeated property lookups.
These getters fetch fromRenderSettingseach time, which could be micro-optimized if invoked frequently within tight loops.
44-46: Avoid magic numbers.
The constants 10.0, 2.0f, 16f, 0.5f, and 0.8f might be more readable as named constants or part of the settings themselves.
99-107: Potential duplication with drawString.
Consider extracting common rendering logic for glyph scaling and positioning into a shared helper to reduce code duplication.
173-173: Magic number 0.7 might need clarification.
Consider referencing a named constant or explaining its origin in a comment for maintainability.
185-257: Complex method structure.
Consider breakingprocessTextinto smaller helper functions to improve readability and maintainability.
265-265: Magic number 8.5 might need clarification.
Consider referencing a named constant or explaining its origin in a comment for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/kotlin/com/lambda/graphics/renderer/gui/font/FontRenderer.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/core/SliderLayout.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/client/RenderSettings.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (25)
common/src/main/kotlin/com/lambda/module/modules/client/RenderSettings.kt (3)
20-21: Adding font-related imports aligns with the new texture API.The imports for LambdaEmoji and LambdaFont support the new font configurations added to the RenderSettings module, which is consistent with the PR's objectives to update fonts and text rendering.
34-35: Well-structured font configuration settings.Adding dedicated settings for text and emoji fonts provides users with more customization options and supports the new font rendering system mentioned in the PR. These settings don't have page visibility conditions, making them always accessible regardless of the selected page.
42-43: SDF parameters properly configured for font rendering enhancement.The new SDF (Signed Distance Field) min/max parameters will help control the sharpness and clarity of font rendering at different scales, which aligns with the PR's goal of "enhancing character clarity at any scale or transformation." The visibility condition ensures these settings only appear on the Font page.
Ensure that these default values (0.4 for min, 1.0 for max) have been tested across different font sizes and scaling scenarios to confirm optimal rendering quality.
common/src/main/kotlin/com/lambda/gui/impl/clickgui/core/SliderLayout.kt (5)
1-16: License header
No issues found with the GPL license header. Everything looks consistent and properly configured.
82-92: Well-managed mouse event handling
The logic for setting the slider’s progress on mouse move and click events is straightforward and properly constrained to the left mouse button. This approach should be robust for typical slider interactions.
93-112: Smooth rendering of progress and outline
The code cleanly splits drawing of the filled area (rect {}) and outline (outline {}), applying animations and color alpha calculations. The operationwidth *= renderProgressis clear, though ensuringrenderProgressstays within[0,1]is important to prevent abnormal visuals.
115-141: DSL methods provide a neat UI builder pattern
The companion object’sslider,sliderBehind, andsliderOvermethods enable flexible composition with other layouts. This approach is consistent with Kotlin DSL best practices and promotes reusability.
64-81:Details
❓ Verification inconclusive
Ensure
bg.widthis non-zero when transforming mouse coordinates
ThedragProgresscomputation leveragesbg.widthin a transform from[0, bg.width]to[0, 1]. Ifbg.widthis ever zero or negative for some reason, the transform might produce unexpected values (NaN or Infinity). Confirm or enforce a minimum width to avoid edge cases.
🏁 Script executed:
#!/bin/bash # Description: Search for calls that set the slider width or affect `bg.width` to confirm it is always > 0. ast-grep --pattern $'bg.width = $_'Length of output: 36
Action Required: Validate and Safeguard bg.width in SliderLayout
The current implementation calculates
dragProgressby mappingmousePosition.xfrom the interval[0, bg.width]to[0, 1]. However, our initial search for explicit assignments or checks that guarantee a positivebg.widthreturned no results. This suggests that the rectangle’s width is implicitly inherited from its parent or default settings. To prevent potentially unexpected transform results (like NaN or Infinity) whenbg.widthis zero or negative, please verify manually that:
- The rectangle defined in the
rect { ... }block (which initializesbg) guarantees a positive width at all times.- If there exists a scenario where
bg.widthmight be zero or negative, a safeguard (such as enforcing a minimum width or performing a conditional check) is in place.Locations to review:
common/src/main/kotlin/com/lambda/gui/impl/clickgui/core/SliderLayout.kt, specifically:
- Lines 64-81 (initialization of
bgand computation ofdragProgress).Please confirm the width’s validity and consider adding an explicit check if necessary.
common/src/main/kotlin/com/lambda/graphics/renderer/gui/font/FontRenderer.kt (17)
22-27: No immediate concerns with these import statements.
30-30: No additional comments.
36-39: Documentation is sufficiently clear.
40-40: Singleton usage makes sense here.
49-57: Documentation is well-structured.
60-65: Default parameter usage is consistent.
No issues found.
66-71: Shader uniform assignments look correct.
73-74: Callback approach is flexible.
No immediate issues spotted.
79-85: Clear documentation of drawGlyph.
86-91: Function signature is clear.
No issues found.
92-98: Shader uniform assignment is repeated here but appears valid.
No concurrency or correctness concerns identified.
109-117: Documentation is consistent with the method's purpose.
118-138: Quad rendering approach is straightforward.
The usage ofgrow(4)ensures sufficient buffer capacity. Implementation looks solid.
140-146: Documentation for getWidth is acceptable.
164-172: Documentation for getHeight is clear.
176-184: Process text documentation is thorough.
259-264: Clear doc for getScaleFactor.
| // Not a great solution | ||
| private val setting = owner as? SettingSlider<*, *> | ||
| private val showAnim get() = setting?.showAnimation ?: showAnimation | ||
| private val hoverAnim get() = setting?.hoverAnimation ?: hoverAnimation | ||
| private val pressedBut get() = setting?.pressedButton ?: pressedButton |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid over-reliance on casting to SettingSlider
Relying on owner as? SettingSlider<*, *> and storing it in setting may introduce subtle bugs if the layout isn't a SettingSlider. The comment “Not a great solution” underscores this. Consider using a design where the slider logic is decoupled from the exact type of the parent or use composition/dependency injection for better separation of concerns.
| /** | ||
| * Calculates the shadow color by adjusting the brightness of the input color. | ||
| * | ||
| * @param color The original color. | ||
| * @return The modified shadow color. | ||
| */ | ||
| fun getShadowColor(color: Color): Color = Color( | ||
| (color.red * RenderSettings.shadowBrightness).toInt(), | ||
| (color.green * RenderSettings.shadowBrightness).toInt(), | ||
| (color.blue * RenderSettings.shadowBrightness).toInt(), | ||
| color.alpha | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential out-of-range color values.
If RenderSettings.shadowBrightness is outside the range [0.0, 1.0], (color.red * RenderSettings.shadowBrightness).toInt() can overflow the 0–255 bounds, potentially causing an exception. Consider clamping the values:
-fun getShadowColor(color: Color): Color = Color(
- (color.red * RenderSettings.shadowBrightness).toInt(),
- (color.green * RenderSettings.shadowBrightness).toInt(),
- (color.blue * RenderSettings.shadowBrightness).toInt(),
- color.alpha
-)
+fun getShadowColor(color: Color): Color {
+ val r = ((color.red * RenderSettings.shadowBrightness).coerceIn(0.0, 255.0)).toInt()
+ val g = ((color.green * RenderSettings.shadowBrightness).coerceIn(0.0, 255.0)).toInt()
+ val b = ((color.blue * RenderSettings.shadowBrightness).coerceIn(0.0, 255.0)).toInt()
+ return Color(r, g, b, color.alpha)
+}📝 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.
| /** | |
| * Calculates the shadow color by adjusting the brightness of the input color. | |
| * | |
| * @param color The original color. | |
| * @return The modified shadow color. | |
| */ | |
| fun getShadowColor(color: Color): Color = Color( | |
| (color.red * RenderSettings.shadowBrightness).toInt(), | |
| (color.green * RenderSettings.shadowBrightness).toInt(), | |
| (color.blue * RenderSettings.shadowBrightness).toInt(), | |
| color.alpha | |
| ) | |
| } | |
| /** | |
| * Calculates the shadow color by adjusting the brightness of the input color. | |
| * | |
| * @param color The original color. | |
| * @return The modified shadow color. | |
| */ | |
| fun getShadowColor(color: Color): Color { | |
| val r = ((color.red * RenderSettings.shadowBrightness).coerceIn(0.0, 255.0)).toInt() | |
| val g = ((color.green * RenderSettings.shadowBrightness).coerceIn(0.0, 255.0)).toInt() | |
| val b = ((color.blue * RenderSettings.shadowBrightness).coerceIn(0.0, 255.0)).toInt() | |
| return Color(r, g, b, color.alpha) | |
| } | |
| } |
| fun getWidth( | ||
| text: String, | ||
| scale: Double = 1.0, | ||
| parseEmoji: Boolean = LambdaMoji.isEnabled, | ||
| ): Double { | ||
| var width = 0.0 | ||
| iterateText(text, scale, false) { char, _, _, _ -> width += char.width + gap } | ||
| return width * getScaleFactor(scale) | ||
| var gaps = -1 | ||
|
|
||
| processText(text, scale = scale, parseEmoji = parseEmoji) { char, _, _, _, isShadow -> | ||
| if (isShadow) return@processText | ||
| width += char.width; gaps++ | ||
| } | ||
|
|
||
| return (width + gaps.coerceAtLeast(0) * gap) * getScaleFactor(scale) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential inaccuracy for multi-line text measurement.
When encountering newline characters, the current logic resets posX but does not track the maximum width per line. The returned value may only reflect the final line’s width.
Consider tracking the maximum line width, for example:
var width = 0.0
+var maxWidth = 0.0
var gaps = -1
processText(text, scale = scale, parseEmoji = parseEmoji) { char, _, _, _, isShadow ->
if (isShadow) return@processText
width += char.width
gaps++
+ maxWidth = maxOf(maxWidth, width + gaps.coerceAtLeast(0) * gap)
}
-return (width + gaps.coerceAtLeast(0) * gap) * getScaleFactor(scale)
+return maxWidth * getScaleFactor(scale)📝 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.
| fun getWidth( | |
| text: String, | |
| scale: Double = 1.0, | |
| parseEmoji: Boolean = LambdaMoji.isEnabled, | |
| ): Double { | |
| var width = 0.0 | |
| iterateText(text, scale, false) { char, _, _, _ -> width += char.width + gap } | |
| return width * getScaleFactor(scale) | |
| var gaps = -1 | |
| processText(text, scale = scale, parseEmoji = parseEmoji) { char, _, _, _, isShadow -> | |
| if (isShadow) return@processText | |
| width += char.width; gaps++ | |
| } | |
| return (width + gaps.coerceAtLeast(0) * gap) * getScaleFactor(scale) | |
| } | |
| fun getWidth( | |
| text: String, | |
| scale: Double = 1.0, | |
| parseEmoji: Boolean = LambdaMoji.isEnabled, | |
| ): Double { | |
| var width = 0.0 | |
| var maxWidth = 0.0 | |
| var gaps = -1 | |
| processText(text, scale = scale, parseEmoji = parseEmoji) { char, _, _, _, isShadow -> | |
| if (isShadow) return@processText | |
| width += char.width | |
| gaps++ | |
| maxWidth = maxOf(maxWidth, width + gaps.coerceAtLeast(0) * gap) | |
| } | |
| return maxWidth * getScaleFactor(scale) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt (1)
51-82: Consider handling direct user input through the text fieldThe current implementation only shows the setting value in the text field but doesn't appear to allow users to directly input values. If this is intentional, it's fine, but if direct text input is desired, you'll need to add handling for user input events.
Additionally, note that the animation will only trigger when the string representation changes. If two different setting values produce the same string output, the animation won't trigger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/EnumSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/NumberSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/client/RenderSettings.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/kotlin/com/lambda/module/modules/client/RenderSettings.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (9)
common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/NumberSlider.kt (3)
28-36: Well-structured generic class designThe
NumberSliderclass has appropriate generic constraints (where V : Number, V : Comparable<V>) and properly handles numeric range values by converting them to Double. This provides type safety while supporting all numeric types that can be used with settings.
37-51: Clean implementation of slider behaviorThe slider implementation properly transforms the setting value to a 0.0-1.0 range for the slider progress and handles the inverse operation when the user interacts with the slider. The use of
roundToStepensures values align with the setting's defined step size.
53-60: Good use of companion object for DSL-style builder functionThe companion object with the
@UIBuilderannotation provides a clean, idiomatic way to create sliders within a layout. The extension function approach follows Kotlin best practices for DSL construction.common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/EnumSlider.kt (3)
26-32: Good inheritance design for enum settingsThe
EnumSliderclass properly extends the baseSettingSliderclass and is constrained to work with enum types, providing a specialized implementation for handling enumerated values.
33-49: Verify boundary behavior when slider is at maximum positionThe calculation
(it * entries.size).floorToInt()could potentially produce an index equal toentries.sizewhen the slider is at exactly 1.0. While thecoerceIncall correctly handles this case, consider using(it * (entries.size - 1)).roundToInt()for more intuitive mapping.The current implementation works because of the bounds check, but the suggested approach might provide more consistent selection behavior across the slider's range.
51-58: Good use of companion object with UIBuilder annotationSimilar to the
NumberSlider, this implementation provides a clean DSL-style builder function. The consistent pattern across different setting types makes the API intuitive to use.common/src/main/kotlin/com/lambda/gui/impl/clickgui/module/settings/SettingSlider.kt (3)
31-39: Well-designed abstract base class for slidersThe
SettingSliderabstract class provides a solid foundation for different types of sliders. The generic parameters are appropriately constrained, and the abstractsettingValueproperty ensures that derived classes properly format their values as strings.
40-50: Good positioning logic for the slider componentThe slider component is properly positioned relative to its parent layout, with clean update logic that keeps the slider in sync with the parent's dimensions.
36-36:Details
❓ Verification inconclusive
Verify animation behavior with always-true condition
The animation setup uses
by animation.exp(0.0, 1.0, 0.5) { true }with a lambda that always returns true. Confirm that this is the intended behavior, as it means the animation will always be considered active.
🏁 Script executed:
#!/bin/bash # Search for other uses of this animation pattern to understand if this is a common pattern rg "by animation\.exp\(.*\) \{ true \}" -A 2 -B 2Length of output: 624
Ensure Intended Always-Active Animation Behavior
The usage of
by animation.exp(0.0, 1.0, 0.5) { true }is unique to this file, which suggests it might be a deliberate design decision to have the animation always active. Please confirm whether an always-true condition is indeed intended forchangeAnimation. If so, consider adding a clarifying comment to document that this behavior is by design.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
common/src/main/kotlin/com/lambda/module/Module.kt (1)
148-162: 🛠️ Refactor suggestionRemove duplicated onEnable/onDisable blocks.
These repeated definitions for enabling/disabling the module produce the same effect multiple times. Consolidate them into a single pair of onEnable/onDisable calls:
--- a/common/src/main/kotlin/com/lambda/module/Module.kt +++ b/common/src/main/kotlin/com/lambda/module/Module.kt @@ -148,15 +148,6 @@ onEnable { playSoundRandomly(LambdaSound.MODULE_ON.event) } - - onDisable { - playSoundRandomly(LambdaSound.MODULE_OFF.event) - } - } - - onEnable { - playSoundRandomly(LambdaSound.MODULE_ON.event) - } - - onDisable { - playSoundRandomly(LambdaSound.MODULE_OFF.event) - }
🧹 Nitpick comments (1)
common/src/main/kotlin/com/lambda/module/hud/ModuleList.kt (1)
36-38: Combine filters for cleaner code.Currently, modules are filtered twice based on
it.isEnabledandit.isVisible.value. Combining these checks into a single filter can slightly improve readability:- val enabled = ModuleRegistry.modules - .filter { it.isEnabled } - .filter { it.isVisible.value } + val enabled = ModuleRegistry.modules + .filter { it.isEnabled && it.isVisible.value }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/src/main/kotlin/com/lambda/module/Module.kt(3 hunks)common/src/main/kotlin/com/lambda/module/hud/ModuleList.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (2)
common/src/main/kotlin/com/lambda/module/Module.kt (2)
120-120: Confirm logic forisVisibledependency.
isVisibledepends onModuleList.isEnabled. IfModuleListis disabled,isVisiblewill always befalse. Verify that this is the intended behavior, especially if modules should remain visible regardless ofModuleList's status.
121-121: Reset property looks great.The reset logic is straightforward. Proceed with caution if there's any data or side effects that should not be reverted automatically.
Related: #79 #78
This pull request is the result of 7 months of hard work
A new maintainable click gui api was implemented
A new setting layout builder was added to create custom gui elements such as buttons, sliders, etc.
The opengl common buffer was reworked to reduce overhead
The texture uploading process was simplified for the end user by providing a new
TextureOwnerobject that keeps track of all the textures created in a given scopeThe functions to upload textures are as follow:
fun Any.upload() = ...fun Any.uploadGif() = ...fun Any.uploadField() = ...A new AnimatedTexture class was added for gif rendering
The texture base class was extended to correctly handle other pixel formats such as BGRA, RG, RED and provide mappings to mojang's native image formats
The pixel buffer was reworked according to the buffer api
Both fonts, text and emojis, were reworked according to the texture api
A new SDF texture (also known as alpha-tested magnification) was implemented in order to display clearer characters at any scale or transformation
There are many breaking changes that need to be corrected before merging
This pull request is NOT ready to be merged
Summary by CodeRabbit
New Features
UI Enhancements
Improvements