Tooltip positioning V3 (aka v1 with default style)

[rswinkle]

This is my very simple take on the discussion/idea here with @sleeptightAnsiC

I only made this a separate branch/pull request in case it significantly diverges but as it is, it's a very small change.

I decided the push/pop wrapper idea didn't make sense and just added 2 members to nk_window_style, initialize them in nk_style_from_table() with the same defaults I was using, and use them for calls to nk_tooltip() and nk_tooltip_begin() instead of hard coding those arguments.

I still say we could/should combine all alignment enums into one at some point if possible, or at least the ones that are literally identical but that's not important right now.

Like you @RobLoach , I just want to get something in. I have too many irons in the fire right now and want to check some off.

[sleeptightAnsiC]

Are you still expecting me to come up with my own version of this? I had quite different ideas (#900 (comment)) but I don't really want to block your work. I didn't thought you would overtake it.

[RobLoach]

This surprised me haha 😆 ... We should likely use the macros in case math isn't there.

Suggested change

	offset.x = radius * cos(accum_time_seconds * speed);
	offset.y = radius * sin(accum_time_seconds * speed);
	offset.x = radius * NK_COS(accum_time_seconds * speed);
	offset.y = radius * NK_SIN(accum_time_seconds * speed);

[rswinkle]

In my defense, I copied that straight from @sleeptightAnsiC though I probably would've forgotten about the macros too.

[sleeptightAnsiC]

lmao

In my defense, I copied that straight from 18 cos/sin symbols that already existed in that file

~/Nuklear $ grep -rn 'cos' ./demo/common/overview.c 
702:                    nk_flags res = nk_chart_push(ctx, (float)cos(id));
713:                nk_tooltipf(ctx, "Value: %.2f", (float)cos((float)index*step));
716:                nk_labelf(ctx, NK_TEXT_LEFT, "Selected value: %.2f", (float)cos((float)index*step));
746:                    nk_chart_push_slot(ctx, (float)cos(id), 1);
760:                    nk_chart_push_slot(ctx, (float)cos(id), 1);
1053:                                nk_chart_push_slot(ctx, (float)cos(id), 1);
1076:                                nk_chart_push_slot(ctx, (float)fabs(cos(id)), 1);
~/Nuklear $ grep -rn 'sin' ./demo/common/overview.c 
383:                 * popup with the ease of use of just using the space for modifiers.
723:                    nk_flags res = nk_chart_push(ctx, (float)fabs(sin(id)));
733:                nk_tooltipf(ctx, "Value: %.2f", (float)fabs(sin(step * (float)index)));
736:                nk_labelf(ctx, NK_TEXT_LEFT, "Selected value: %.2f", (float)fabs(sin(step * (float)col_index)));
745:                    nk_chart_push_slot(ctx, (float)fabs(sin(id)), 0);
747:                    nk_chart_push_slot(ctx, (float)sin(id), 2);
759:                    nk_chart_push_slot(ctx, (float)fabs(sin(id)), 0);
761:                    nk_chart_push_slot(ctx, (float)sin(id), 2);
1052:                                nk_chart_push_slot(ctx, (float)fabs(sin(id)), 0);
1063:                                nk_chart_push_slot(ctx, (float)fabs(sin(id)), 0);
1075:                                nk_chart_push_slot(ctx, (float)fabs(sin(id)), 0);
1077:                                nk_chart_push_slot(ctx, (float)fabs(sin(id)), 2);

[RobLoach]

Text height may actually make sense here. What if your layout's min-height is 0? Perhaps we use the largest of either one?

[rswinkle]

Wouldn't a 0 min_height mess everything up anyway? Unless they were just doing it as a way to force themselves to always manually set row heights. Speaking of, what is with this:

    if (height == 0.0f)
        layout->row.height = NK_MAX(height, layout->row.min_height) + item_spacing.y;

This is the only other place row.min_height is used outside of the getter/setter and my function. Why are
we taking the MAX() of 0 and another number? Can I go ahead and change it?

Anyway, I'll test it with a 0 min_height but how about
NK_MAX(win->layout->row.min_height, ctx->style.font->height+2*win->padding.y);

because I don't think the popup functions add any padding, just use the rect directly.

[RobLoach]

NK_MAX(win->layout->row.min_height, ctx->style.font->height+2*win->padding.y);

Sure, may be better than making a guess on min-height 👍

[rswinkle]

UPDATE: yeah we don't have to worry about the min height being 0. nk_layout_reset_row_min_height() gets called in nk_panel_begin() which happens inside nk_popup_begin() so which means that there isn't really a way for the user to set the min row height for any type of popup window unless they use the window_type_begin() call and window_type_end() directly (some types only have begin/end cals, like contextual, but others like tooltip are almost always used with the higher level wrappers because no one wants to deal with calculating the text dimensions. I had to do this to create a problem:

            if (nk_input_is_mouse_hovering_rect(in, bounds)) {
                /*nk_tooltip_begin(ctx, "This is a default tooltip");*/
                if (nk_tooltip_begin(ctx, 200)) {
                    nk_layout_set_min_row_height(ctx, 0);
                    nk_layout_row_dynamic(ctx, 0, 1);
                    nk_label(ctx, "This is a default tooltip", NK_TEXT_LEFT);
                    nk_tooltip_end(ctx);
                }
            }

And neither version fixed it because it doesn't actually use our height:

    bounds.x = (float)x;
    bounds.y = (float)y;
    bounds.w = (float)w;
    bounds.h = (float)nk_iceilf(nk_null_rect.h);

This was there from the original tooltip code so I left it. The height we're talking about is just used for calculating the position based on the enum, so in the above worst case, we just moved it to the correct spot, but it's still a skinny rectangle with no visible text:

I don't know enough about how the windows are calculated and laid out to confidently set bounds.h to our height, even if it appears to work. I'll leave the MAX in because at least it fixes the positioning but beyond that I don't think there's much else I should do.

[rswinkle]

Are you still expecting me to come up with my own version of this? I had quite different ideas (#900 (comment)) but I don't really want to block your work. I didn't thought you would overtake it.

Sorry, I'm working on a project where Nuklear related items are making up more and more of my TODO list and I just really want some things in. My original "good idea" response in that thread was more about the global styling default/ability than any other major change so it made sense to just go ahead with such a small change.

How about we abandon the approach of creating new nk_tooltip_* altogether?
Instead, how about we introduce a new styling struct specific to tooltip?
I feel like having a function permutation will never be enough,
and I'll be fine with having enum inside of styling struct.

I actually have a comment about making a tooltip style struct but currently all window type styling is included in nk_style_window, which is faster and more convenient given the way we use them (no need to discriminate with a tag, the member names are the discrimination). Also if we did break up all window types into their own styling structures, it would only be 3-5 members a piece which doesn't seem worth it.

Right now for tooltip even with my additions, it's only:
border_color
border
padding
origin
offset

And others only have the first 3.

Not sure what else we'd need to add to make it more sensible to break out the structure, let alone all of them.

As for the keeping the nk_tooltip_offset() functions, think about it. Which is better (and the user would have to know about these functions and that nk_flags (a uint32) would work for an enum, which even I'm not 100% sure is guaranteed to work in general even if it works here.

nk_style_push_flags(ctx, &ctx->style.window.tooltip_origin, NK_BOTTOM_CENTER);
nk_push_vec2(ctx, &ctx->style.window.tooltip_offset, nk_vec2(0, -12));
nk_tooltip(ctx, "text");
nk_pop_flags(ctx)
nk_pop_vec2(ctx)

vs

nk_tooltip_position(ctx, "text", NK_BOTTOM_CENTER, nk_vec2(0, -12));

And if you write a wrapper to make in convenient for the user it either uses the local stack to save/restore, or it literally uses the push/pops which is more painful and less efficient. Either way, the function nk_tooltip_position() still exists. So why not just leave it the way it is and bypass the styling altogether in this case?

As for tooltips (and other popups) going outside the nuklear draw space, I have some ideas for that that seem relatively simple and don't affect anything here. There might be the option of additional styling but it would be a single additional member (possibly per-popup type) but my first pass would even have that, it would just do some sane default behavior to prove the concept.

[RobLoach]

I believe we can roll with this, and improve it as needed in follow ups. Anything else you would like to get in prior?

[rswinkle]

I believe we can roll with this, and improve it as needed in follow ups. Anything else you would like to get in prior?

Not that I can think of. If you agree it's good to go, LGTM.