Upgrade inspector with new capabilities and detached window mode. #86
Conversation
…r to avoid dependence on global states
…to avoid dependence on global states
… avoid dependence on global states
…tion inside the inspector
…he property to be set
…w mode. The Inspector can now manage states from components and constraints and measure distances in a UI. It also now has the capability to run in a detached mode instead of an overlay and has many new hotkeys for quickly performing functions.
There was a problem hiding this comment.
Read the comment on Elementa.api and the one on InspectorManager.kt before all else, they require significant changes which very likely affect how you implement other, smaller changes.
I will review the awt/glfw classes once above comment has been addressed.
I have not yet reviewed the new *Tab classes and other related content-specific changes in the Inspector, I'll do that in round two.
| @@ -137,6 +145,7 @@ class Window @JvmOverloads constructor( | |||
| } | |||
| } | |||
| } | |||
| currentWindow.set(null) | |||
There was a problem hiding this comment.
This must be in a finally so it is always unset, even when an exception occurs in the catch block.
| @@ -59,6 +66,7 @@ class Window @JvmOverloads constructor( | |||
| private fun doDraw(matrixStack: UMatrixStack) { | |||
| if (cancelDrawing) | |||
| return | |||
| currentWindow.set(this) | |||
There was a problem hiding this comment.
This must be at the start of the try block below, otherwise it won't be un-set if an exception occurs before that block (e.g. requireMainThread, or one of the renderOperations).
It's also semantically wrong to have this set while renderOperations are being drained because renderOperations is global and not specific to this window.
| @@ -1237,8 +1237,9 @@ abstract class UIComponent : Observable() { | |||
| /** | |||
| * Hints a number with respect to the current GUI scale. | |||
| */ | |||
| @Deprecated("This relies on global states", replaceWith = ReplaceWith("guiHint(number, roundDown, component)")) | |||
There was a problem hiding this comment.
I've said this before but I guess I'll have to repeat myself:
I don't think we should at this point deprecate this method (as well as the roundToRealPixel ones) because Kotlin's context receiver feature may make the new methods way more ergonomic to use than they currently are, and therefore I don't think we should at this point lock them into the API (and we don't need to anyway because atm the whole resolution manager thing is only used by the inspector, not by any API user).
Don't delete the line though, just comment it out. We will probably need it at some point, and this way it already serves as a hint for internal usages.
| private var unroundedScissorBounds: ScissorBounds? = null | ||
| private var roundedScissorBounds: ScissorBounds? = null |
There was a problem hiding this comment.
None of these need to be mutable and you don't need to manually handle initialization state tracking.
To get rid of the mutability of the first field, simply make the primary constructor private and make the field an argument there (and then create a new secondary constructor matching the current primary one for backwards compatibility).
To get rid of the mutability of the second field, simply initialize it via lazy.
| get() = Window.ofOrNull(this) | ||
|
|
||
| internal val UIComponent.resolutionManager: ResolutionManager | ||
| get() = this.window?.resolutionManager ?: DefaultResolutionManager |
There was a problem hiding this comment.
Unnecessary this
(same for the other manager extension methods)
| /** | ||
| * The inspector manager provides debug utilities for debugging with the [Inspector] or with a custom debug component. | ||
| */ | ||
| object InspectorManager { |
There was a problem hiding this comment.
I don't think this should exist.
It unnecessarily adds a bunch of global state into stuff which doesn't conceptually need to be global, like at all. Why can't every Inspector just manage this by itself?
I also don't think the Inspector should be removed from its window while it is being displayed externally. That makes it incompatible with existing user code which will add/remove the inspector to enable/disable it. Instead, the inspector content should be separated from the Inspector, and then the Inspector can parent its content either to itself, or to the external window as required (that'll also fix the issue where if you move the inspector and then detach it, it'll render offset in the external window as well; plus it doesn't follow the window size).
| /** | ||
| * Adds a custom debug component either directly to [window] or to an external display | ||
| * depending on whether the current session is external or not. If no debug instance exists, | ||
| * then a new one will be created from on the current value of [startDetached] | ||
| */ | ||
| fun addCustomDebugComponent(window: Window, component: UIComponent) { | ||
| val openDisplay = debugSessions[window] | ||
| if (openDisplay == null) { | ||
| createExternalDisplay(window, component) | ||
| } else { | ||
| openDisplay.components.add(component) | ||
|
|
||
| if (openDisplay.isExternal) { | ||
| openDisplay.display.addComponent(component) | ||
| } else { | ||
| window.addChild(component) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Removes a custom debug component from the debug session of the supplied [window]. | ||
| * If the result of this operation leaves no debug components, then the debug session is closed. | ||
| * | ||
| * If no debug session exists, then nothing will be done. | ||
| */ | ||
| fun removeCustomDebugComponent(window: Window, component: UIComponent) { | ||
| val session = debugSessions.remove(window) ?: return | ||
| session.components.remove(component) | ||
| if (session.isExternal) { | ||
| session.display.removeComponent(component) | ||
| } else { | ||
| window.removeChild(component) | ||
| } | ||
| if (session.components.isEmpty()) { | ||
| cleanupWindow(window) | ||
| } | ||
| } |
There was a problem hiding this comment.
How's this supposed to work? If it's just added to the external display, then won't it necessarily get in the way of the inspector and vice versa? Would it not make more sense to give custom components their own window, or a dedicated space in the existing inspector UI? Do we even need to support custom debug components? That's one ginormous behavioral api surface we're exposing if we do, I would very much prefer we don't; at least we should put it behind a OptIn so we don't have to provide backwards compatibility.
| @@ -87,3 +92,147 @@ fun <T> State<T>.onSetValueAndNow(listener: (T) -> Unit) = onSetValue(listener). | |||
|
|
|||
| operator fun <T> State<T>.getValue(obj: Any, property: KProperty<*>): T = get() | |||
| operator fun <T> State<T>.setValue(obj: Any, property: KProperty<*>, value: T) = set(value) | |||
|
|
|||
| fun State<String>.empty() = map { it.isBlank() } | |||
There was a problem hiding this comment.
emptyseems awfully specific and its implementation is trivial (yet, why is itisBlank, notisEmpty? no particular reason). I don't think that belongs in the base elementa api (or in any api with that name).
| val halfPixel = 0.5f / UResolution.scaleFactor.toFloat() | ||
| val mouseX = UMouse.Scaled.x.toFloat() + halfPixel | ||
| val mouseY = UMouse.Scaled.y.toFloat() + halfPixel |
| import gg.essential.universal.USound | ||
| import java.awt.Color | ||
|
|
||
| internal class CompactToggle( |
There was a problem hiding this comment.
As is, I've got absolutely no clue which way round is On vs Off. They look exactly the same, just mirrored.
There was a problem hiding this comment.
Any suggestions for an updated design to make it more clear?
…ged` and `State` in names
…yName function to be passed
…uerying awt on Minecraft thread
|
Comments should now be addressed except for the distance measurement. I will take a look at the unreviewed code to see if I can find anything to change before you take a look at it,. |
# Conflicts: # src/main/kotlin/gg/essential/elementa/markdown/drawables/TextDrawable.kt
The Inspector can now manage states from components and constraints and measure distances in a UI. Additionally, it can run in a detached mode instead of an overlay and has many new hotkeys for quickly performing functions.