uilist-rewrite-wip #23
Conversation
|
|
||
| // TODO: why is this here |
There was a problem hiding this comment.
Translation: i find it weird that hotkeys get reassigned each time the window is resized. It feels like this logic should happen elsewhere, but I haven't thought about this enough to say anything more constructive.
There was a problem hiding this comment.
Heh, now that you mention it that is pretty dumb. We can move that somewhere more appropriate, I am sure.
| ImVec2 available_bounds = ImGui::GetMainViewport()->Size; | ||
| available_bounds.x *= 0.9; // for aesthetic reasons | ||
| if( desired_bounds.has_value() ) { | ||
| // TODO: this should never happen actually, and is a bad API |
There was a problem hiding this comment.
translation: desired_bounds is used as both input parameter (i.e. things tell us where they want the window to be placed) and output parameter (things (which ones? i forgor) check this to see where we end up being placed?)
It also uses magical -1.0 to mean "i don't care, use a good default", which can not be distinguished for "please do it at -1.0 pixels". Oh and negative values also have some nontrivial meaning which i forgor again.
Basically the code around the if( !both_neg ) { line is tremendously confusing.
I think that
- it should not be an output parameter at all (we can make a different out parameter if neccessary, but also I think we already have the window size stored and accessible somewhere) and
- it should use std::optional instead of magic -1.0 constant
There was a problem hiding this comment.
The magic −1.0 is part of the ImGui API. std::optional didn’t exist (or wasn’t widely available) back when ImGui was first getting started. Plus, there is the small problem that std::optional takes more space than a simple float. An ImOptionalVec4 struct with four optional values would be sparse! That’s terrible for cache locality. Probably not enough to matter in practice, but we would suffer just from looking at it. I would not disagree with you if you pointed out that he could have used a NaN value instead of −1.0, but I guess he didn’t think of it.
Note that the values between 0.0 and 1.0 are also special. They don't make sense as pixel values, so we use them to represent a percentage of the viewport size instead (see cataimgui::window::draw()).
The !both_neg is rather confusing. I’ve had to think for several minutes to recall why I wrote it that way. If you look at uilist_impl::get_bounds() you’ll see that it does parent.desired_bounds.value_or( parent.calculated_bounds ). So if desired_bounds has a value, we are going to give it to ImGui when we create our window. If desired_bounds.w and desired_bounds.h are both negative then we will give them to ImGui that way. But if only one of them is negative or neither of them are then we will overwrite them with our calculated values first.
We could of course change that if there’s a better way.
There was a problem hiding this comment.
Plus, there is the small problem that std::optional takes more space than a simple float. An ImOptionalVec4 struct with four optional values would be sparse! That’s terrible for cache locality
I think this is all completely irrelevant because desired_bounds should never be in the hot path. It should only be read here, in calc_data (that does loads of things that are much more heavy that unwrapping an optional), and never since.
I totally agree that the actual bounds (calculated with input from desired_bounds, but stored separately) should be four floats (xywh), but that's different!
NaN
NaN comparison is even more tricky than float comparison. I strongly believe we should just use optional and not try to be clever.
If you look at
uilist_impl::get_bounds()you’ll see that it doesparent.desired_bounds.value_or( parent.calculated_bounds ).
It should just be parent.calculated_bounds, and desired_bounds should never be consulted by this point.
I admit I haven't thought this through any further than this, and it might not be as straightforward, but that was my train of thought at the time.
There was a problem hiding this comment.
isnan is pretty easy to use, but the main point is that I am doing what ImGui is doing.
But yes, we could just always give the calculated_bounds to ImGui, having already copied whatever we needed from desired_bounds.
| bool has_titlebar = !title.empty(); | ||
| if( has_titlebar ) { | ||
| // | ||
| // TODO: calc_size does not account for items spacing |
There was a problem hiding this comment.
Other TODOs are "nice to have" but this one is the current blocker.
I failed to figure out the exact padding of the elements, and they don't quite fit (or, alternatively, reserve too much space). Changing the spacing at runtime via ImGui debug menu should demo the issue.
Small part of the problem / minor pointer is that we create a new single-cell table that holds the entire (entire!) uilist in it for some aesthetic reasons, but the way that cell padding interacts with the other cell paddings is unobvious.
There was a problem hiding this comment.
The table is actually three columns, each containing a single cell. The middle column holds the menu. The left and right columns reserve space on the left and right respectively so that the caller can add their own UI in that space.
| ImVec2 label_size = ImVec2(); // first column, label | ||
| ImVec2 label2_size = ImVec2(); // second column, optional (?) | ||
|
|
||
| // TODO: this is bad |
There was a problem hiding this comment.
translation: I have not thought about the extra_space_left / extra_space_right at all, and might be mishandling this badly. I also don't understand (read: have done zero research) why we need this at all.
| desc_enabled = false; // TODO: really? | ||
| } | ||
|
|
||
| // TODO: what's up with header/footer horizontal padding? |
There was a problem hiding this comment.
idk, short-term debug comment probably
| desc_size.x = std::max( desc_size.x, entry_desc_size.x ); | ||
| } | ||
| if( desc_size.x == 0 ) { | ||
| desc_enabled = false; // TODO: really? |
There was a problem hiding this comment.
translation: maybe there's a better way to disable it than checking that the calculated width is 0 (it might also be fine, idk. It's a minor thing)
A branch that rewrites uilist to support text wrapping.
This is a pull request just to facilitate github comments.
I'm making no promises that I'll work on this in any near future.
Feel free to pick it up if you wish