godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Fix various issues with SplitContainer that make scripting it difficult. #6230

Open Koyper opened 1 year ago

Koyper commented 1 year ago

Describe the project you are working on

GUI elements for handling large collections of items, and GUI elements for organizing related containers of text, lists and collections - common GUI utility used frequently.

Describe the problem or limitation you are having in your project

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The code is complete per PR #72680

The PR combines this proposal #6231 for two enhancements that add begin and end margins to the drag area, and exposes the drag area Control to scripting.

https://user-images.githubusercontent.com/33969780/216714121-eac52fb5-6df7-41b5-8239-57fa96f51419.mov

https://user-images.githubusercontent.com/33969780/216715235-b7f8445b-695a-4f8a-baec-484b1016f809.mov

If this enhancement will not be used often, can it be worked around with a few lines of script?

No. The script required to work around these difficiencies runs to hundreds of lines of unreliable script. The _ready notification issue can't be worked around at all, yields and awaits are required to try and guess when the children will be ready.

The new SplitContainer per the PR does the same thing better with just a few lines of script that anyone can understand.

Is there a reason why this should be core and not an add-on in the asset library?

The improvements can't be added via an add-on.

YuriSizov commented 1 year ago

There is no background color or StyleBox for the background of the split bar, so clunky scripting has to follow the split_offset to keep a line centered in the split bar; If you wanted the background behind the grabber icon, you would have to draw it behind the entire SplitContainer.

The new drag area that can be wider than the split bar has no facility to offset it to avoid it overlaying an adjacent scroll bar, so themes that have a scrollbar hard up against the right side of their container will be blocked by the grab area. This means that themes with a hairline split bar or a collapsed split bar can't take advantage of the purpose of the separate grab area.

These two issues aren't related to the subject of this proposal. This is about more styling or theming options, not scripting SplitContainer. Should be made into a dedicated proposal (and a dedicated PR, if accepted).

SplitContainer has no reliable method or signal for notification of when initialization of child controls is ready. _ready() is called earlier than the child containers are resized for the split offset.

This is not unique to split containers. Any container resizes their children, so if existing tools don't give users enough control/information over the state of the control, then a shared solution must be proposed and implemented, on the Container level. That said, containers already report when sizing changes, and whether it happens for the first time or for the Nth time doesn't seem relevant to me, your code should react to either case the same way and adjust whatever custom controls that you have accordingly.

Either way, this needs to be split into a dedicated, all-Container proposal too, with a proper solution.

There is no signal for when the split drag is released or started, so a very common use of saving and restoring the user's split offsets in a resource has to be timed off when the dragged signal stops.

This could be nice to have, feel free to open a PR for that.

While developing a script with the wider grab area, you can't easily visualize the area for testing purposes, without more inconvenient draw code to track the split offset and grab area Control, which is not exposed for scripting.

Well, that's pretty normal for debug drawing. But isn't your other proposal about exposing a way to access the grab area control? And as a small QoL I think you can make a PR to expose that control already, without it being linked to any other enhancement.

The improvements can't be added via an add-on.

No, but for the sake of argument, a custom SplitContainer can be implemented as an addon, as Godot exposes the necessary logic to create custom containers.

Koyper commented 1 year ago

There is no background color or StyleBox for the background of the split bar, so clunky scripting has to follow the split_offset to keep a line centered in the split bar; If you wanted the background behind the grabber icon, you would have to draw it behind the entire SplitContainer.

The new drag area that can be wider than the split bar has no facility to offset it to avoid it overlaying an adjacent scroll bar, so themes that have a scrollbar hard up against the right side of their container will be blocked by the grab area. This means that themes with a hairline split bar or a collapsed split bar can't take advantage of the purpose of the separate grab area.

These two issues aren't related to the subject of this proposal. This is about more styling or theming options, not scripting SplitContainer. Should be made into a dedicated proposal (and a dedicated PR, if accepted).

Acknowledged. Please see my remarks at the bottom regarding breaking out the PR features.

SplitContainer has no reliable method or signal for notification of when initialization of child controls is ready. _ready() is called earlier than the child containers are resized for the split offset.

This is not unique to split containers. Any container resizes their children, so if existing tools don't give users enough control/information over the state of the control, then a shared solution must be proposed and implemented, on the Container level. That said, containers already report when sizing changes, and whether it happens for the first time or for the Nth time doesn't seem relevant to me, your code should react to either case the same way and adjust whatever custom controls that you have accordingly.

What's unique about split containers, is that the split offset reference of 0 is based off the sizing of the first control. I would not have chosen to do that because it would be less complex to have based the reference from the origin of the split container and let the children size into the proportioned sides. Prior to the recent PR (#65028) that fixed some issues with split container sizing, I tried every trick in the book at the scripting level to be notified the containers were ready without happiness. I think it was two lines of code to emit that signal at the point the split container knows the sizing, so call it a QoL improvement, it is super handy in comparison to polling the two child controls and combining their behavior to figure out when it's ready. While that PR might have sorted some of that out, I would argue that no one should have to struggle with obtaining this otherwise easy-to-provide signal. Either way, this needs to be split into a dedicated, all-Container proposal too, with a proper solution.

There is no signal for when the split drag is released or started, so a very common use of saving and restoring the user's split offsets in a resource has to be timed off when the dragged signal stops.

This could be nice to have, feel free to open a PR for that.

Acknowledged.

While developing a script with the wider grab area, you can't easily visualize the area for testing purposes, without more inconvenient draw code to track the split offset and grab area Control, which is not exposed for scripting.

Well, that's pretty normal for debug drawing. But isn't your other proposal about exposing a way to access the grab area control? And as a small QoL I think you can make a PR to expose that control already, without it being linked to any other enhancement.

You are right that should the grab area Control be exposed, debug drawing can be scripted. I would argue however, that while designing an interface, you will repeatedly want to turn the debug drawing on and off and all over the place. Does this kind of convenience fall into a gray area?

The improvements can't be added via an add-on.

No, but for the sake of argument, a custom SplitContainer can be implemented as an addon, as Godot exposes the necessary logic to create custom containers.

The issue for me is that I am using these improvements in my project and need them integrated in the engine. They surely can remain custom to my project, but I thought they would benefit others.

I am still learning the godot PR culture, and would like to see if this approach for cherry picking this split container PR is possible: It would be far easier for me to strip out the features deemed a bell or whistle, than to create multiple PRs of each bit and piece, all of which were developed as a group and rely on the same code improvements and simplifications I made, the entirety of which runs to 108 lines of code in the cpp file. I also made sure it is a backward-compatible drop-in replacement.

What if I were to submit a new PR with all the "core" modifications, minus the minor enhancements that can easily be added back once the core PR were merged?

How do you prefer to handle extensive enhancements to a class that are simply impractical to break out into small bits?

YuriSizov commented 1 year ago

What's unique about split containers, is that the split offset reference of 0 is based off the sizing of the first control.

Other containers also include children sizes into their sizing decisions. Like box or flow containers. I don't follow the rest of your argument, but I'm not saying that the signal would be totally useless, just that the split container is in no way unique and it doesn't make sense to handle it specifically for the split container, ignoring the rest. And I still stand by that there is nothing special about the first resize compared to the possible future resizes, when the split factor or the children sizes change.

If you make a dedicated proposal, we can discuss it properly, with examples and necessary details. We can't really continue a coherent discussion, and hope others to follow, with these large multifaceted comments with large quotes, can we?

You are right that should the grab area Control be exposed, debug drawing can be scripted. I would argue however, that while designing an interface, you will repeatedly want to turn the debug drawing on and off and all over the place. Does this kind of convenience fall into a gray area?

There is no gray area here. The same argument can be made about a lot of things, but we typically don't provide such debug rendering for controls. You can easily create your own drawing logic for things that you need, we just need to provide the necessary references.

There is also a proposal for generic debug draw tools for 2D and 3D where something like that may become a reality at some point, but for now I don't think there is a good argument to add this specifically to the split container.


RE: PR Culture.

PRs should solve specific issues. If some issues are connected, they can be solved together, some general code enhancement in the areas touched by a PR can also be performed together with the main changes. However, you present us here with a set of multiple unrelated features, where only relation between them is that you need them in your project. That's not something that makes them a good bundle inclusion to the engine though.

You've identified several points of enhancement or of pain. I can see that some of them can be clearly addressed with standalone, small, easy to verify, review, and accept PRs. Some of them don't need much discussion, can be included in the next release, and be done with. Others are totally arbitrary, and may or may not benefit most users. How are we supposed to gather feedback here? Are users supporting your idea because they like some of the features, or because they love all of them? Or the other way around, do they avoid upvoting the proposal because while they love some, they don't need the others? And again, would this discussion be easy to follow here?

I mean, sure, in the end, if several of your ideas are accepted a single PR can be made to close them all. But that will take some time anyway, as this is a request for enhancement that hasn't even started to be discussed. While other changes (like signals for dragging, a getter for the grabber control) can be made now, tested and included now, and don't need a very long discussion. There is simply no point in putting everything in one basket and making everything depend on each other. We don't take enhancements wholesome, each needs to be accepted.

So for the sake of discussion, there needs to be dedicated proposals. For the sake of providing useful features to our users, it makes sense to implement the accepted parts when they are accepted, instead of waiting for your entire grand idea to be accepted. It may never be fully agreed upon.

Koyper commented 1 year ago

I appreciate the considered arguments and we can move along when I follow up with a compacted PR. This is a good discussion for me and will improve my future PR's. I would never deliberately suggest that anyone needs a feature because I do, just to be clear about that.

I'm still struggling with the assertion that these features are unrelated - they are related because together they fix all the issues known to me in the split container that in my judgement only, would also help others. Nevertheless, I do of course understand how code reasoning is easier in small bits. Furthermore, I'm a new contributor with no track record that you can rely on.