Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for 9-slice/9-patch -> merge with latest upstream version + remove build.py related code #304

Merged
merged 14 commits into from
Aug 17, 2021

Conversation

lautriva
Copy link
Contributor

It seems that the PR #69 is abandoned, but the 9patch feature would be nice to have in Nuklear and as @Hejsil suggests, it look like very close to validation 🙂
So I've done the merge to latest master commit (at the time of writing) + removed changes related to build.py

No changes have been made on @Michael-Kelley's code 🙂

I think that Nuklear could be used also as an end-user GUI and not only as developer's one and I think this PR is a nice step toward it
(And I'm more convinced when I saw the last demo screenshot with the army-fps-ish options menu)

@dumblob
Copy link
Member

dumblob commented May 31, 2021

@FrostKiwi could you maybe find some time for this? There seems to be quite some demand to prioritize this one and I'm still out of time for this project 😢.

@FrostKiwi
Copy link
Contributor

@FrostKiwi could you maybe find some time for this? There seems to be quite some demand to prioritize this one and I'm still out of time for this project .

Will dive into it :]

Copy link
Contributor

@Hejsil Hejsil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took a look over it again. Haven't tested it, but the code looks good 👍 I've left some nitpick comments :)

{
struct nk_9slice s;
nk_zero(&s, sizeof(s));
struct nk_image *i = (struct nk_image*)&s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the cast when we could just do struct nk_image *i = &s.img; ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get this changed, @dav64 ? It makes the code less clear, as it is, IMO.
Other that this, the rest of the change looks good.
It would be good to have a usage example in the demos -- @FrostKiwi, are you still up for writing a new skinning example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After code investigating, I didn't found if there were specific reason for the original author (Michael) to not using something like @Hejsil solution as it also look cleaner to me
Maybe that only was Nuklear casting fashion at the time of writting 🤔
I'll update that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, let me know if you need anything else 🙂

{
struct nk_9slice s;
nk_zero(&s, sizeof(s));
struct nk_image *i = (struct nk_image*)&s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

{
struct nk_9slice s;
nk_zero(&s, sizeof(s));
struct nk_image *i = (struct nk_image*)&s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here

{
struct nk_9slice s;
nk_zero(&s, sizeof(s));
struct nk_image *i = (struct nk_image*)&s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here

{
struct nk_9slice s;
nk_zero(&s, sizeof(s));
struct nk_image *i = (struct nk_image*)&s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even here

{
struct nk_9slice s;
nk_zero(&s, sizeof(s));
struct nk_image *i = (struct nk_image*)&s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's everywhere! 😱

Comment on lines 101 to 112
switch(background->type) {
case NK_STYLE_ITEM_IMAGE:
nk_draw_image(out, *bounds, &background->data.image, nk_white);
break;
case NK_STYLE_ITEM_9SLICE:
nk_draw_9slice(out, *bounds, &background->data.slice, nk_white);
break;
case NK_STYLE_ITEM_COLOR:
nk_fill_rect(out, *bounds, style->rounding, background->data.color);
nk_stroke_rect(out, *bounds, style->rounding, style->border, style->border_color);
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we have this switch in quite a few places. I wonder if an nk_draw_background function would be something nice to have 🤔

@FrostKiwi
Copy link
Contributor

Learning to properly use skinning in this new context took me forever and properly exporting skinning designs from Photoshop has also pitfalls.
Next PR after this should be an updated documentation :S
Have an exam coming up this week, will give my review with some designs to test on next week.

@lockie
Copy link
Contributor

lockie commented Aug 5, 2021

Is there a chance this would be merged anytime soon?

@dumblob
Copy link
Member

dumblob commented Aug 7, 2021

@lockie we strongly prefer review of more than one person but here only @Hejsil found his precious time to review it so far (I'm too busy for this 😢). If you could review it, we'll immediately merge this.

And if you'll review few other PRs, I'll give you full rights to this repo afterwards as we need more of interested people.

@lockie
Copy link
Contributor

lockie commented Aug 7, 2021

@dumblob thank you for your trust! I'll have a deeper look into the code of this PR a bit later.

@lockie
Copy link
Contributor

lockie commented Aug 14, 2021

I've took the time to review this patch, and besides the possible issue with naming and language bindings, it is pretty straightforward and LGTM 👍

@@ -480,7 +480,8 @@ struct nk_rect {float x,y,w,h;};
struct nk_recti {short x,y,w,h;};
typedef char nk_glyph[NK_UTF_SIZE];
typedef union {void *ptr; int id;} nk_handle;
struct nk_image {nk_handle handle;unsigned short w,h;unsigned short region[4];};
struct nk_image {nk_handle handle; nk_ushort w, h; nk_ushort region[4];};
struct nk_9slice {struct nk_image img; nk_ushort l, t, r, b;};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming here might be problematic for other language bindings. In those the nk_ prefix is usually removed and moved to e.g. package name (or whatever target language provides), so in those bindings we got something like nk.image instead of nk_image. I'm author of Common Lisp bindings, and there probably wouldn't be the problem there - CL allows for names like 9slice, but other languages might not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename to scale9 or grid9 or slice9?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like nine_slice might do the trick as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed 9slice to nine_slice
Let me know if you need anything else 🙂

David B added 2 commits August 17, 2021 18:54
Prevents breaking other languages bindings when nk_ is replaced as a namespace and identifiers cannot start with a digit
Copy link
Contributor

@lockie lockie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it is good to merge 🙂

@Hejsil Hejsil merged commit 6322f53 into Immediate-Mode-UI:master Aug 17, 2021
@Hejsil
Copy link
Contributor

Hejsil commented Aug 17, 2021

Nice!

sthagen added a commit to sthagen/Immediate-Mode-UI-Nuklear that referenced this pull request Aug 19, 2021
@FrostKiwi
Copy link
Contributor

@Michael-Kelley @dav64
Anyone know what nk_ushort l, nk_ushort t, nk_ushort r, nk_ushort b stands for? It's a public API, but not documented as to how you are supposed to use it. Anytime you interact with anything 9-slice you have to specify it. I figured it's the square corner size of the 9 slice and have treated it as such, however, that doesn't actually seem to be quite true.

What does l t r b stand for? Would love to properly document it.

@lautriva
Copy link
Contributor Author

If I remember correctly, it stands for Left, Top, Right and Bottom

But I cannot remember exactly the specifics/inner working as its been a while since I didn't used Nuklear

Hope this helps

@Ashe
Copy link

Ashe commented Mar 7, 2023

Funnily enough I found myself here also needing help with this.

The letters are indeed top-left-right-bottom, but to my surprise the entire image is used in each segment of the nineslice.

I was EXPECTING it to say 'the top t pixels of the image is used for the top 3 segments of the nine-slice', but actually it's 'the entire texture is stretched as each part of the nineslice'

It looks very bizarre, maybe I'm missing something but I don't know how this is supposed to be used other than

                nk_nine_slice slice;
                slice.img = *skin->Get();
                slice.b = 50;
                slice.t = 50;
                slice.l = 50;
                slice.r = 50;
                m_context->style.window.fixed_background = nk_style_item_nine_slice(slice);

This results in a copy of the image being in the top-left corner as a 50x50 image, then a stretched image of 50 px in height at the top, then another 50x50 in the top right.. Same for all the other sides.

@revolucas
Copy link

Seemed to work when I tested it. I couldn't get it to look right with smaller resolution images I created but had it working with the example image from here: https://devforum.roblox.com/t/9-slice-editor-studio-beta/1534739

nineslice

Are you setting the image width, height and region property in your 'Get'?

@Ashe
Copy link

Ashe commented Mar 8, 2023

Get is just returning my nk_image, which I create in my object like so:

image = std::make_unique<struct nk_image>(nk_image_id(*textureID));

So yeah, you're right I didn't set the width/height/region, whoops. Crazy that images render fine without them though!.

What is 'region'? Could you give me a snippet of code for where those values come from?

EDIT: Thanks for your reply by the way!

EDIT EDIT: I got it working. Here's what they represent for future readers:

region[0]: x-coordinate of the top-left corner of the sub-region
region[1]: y-coordinate of the top-left corner of the sub-region
region[2]: width of the sub-region
region[3]: height of the sub-region

The region array in nk_image represents the sub-region of the image that should be displayed, if you don't specify the region values, they default to zero, which means that the entire image is displayed.

However, if you set the w and h values of the nk_image to something other than zero, it means that the image is expected to have a different size than its actual size. This is useful for cases where you want to scale an image up or down, or when you want to display only a part of a larger image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants