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

introduce tiling window background, fix nk_image_is_subimage() #444

Closed

Conversation

FrostKiwi
Copy link
Contributor

grafik
This works by modifying the UVs to sample the texture at 1:1 pixel size. It also assumes, that the backend will repeat the texture, instead of Nuklear drawing a bunch of repeated nk_images. As such, for the instance with OpenGL, you should configured the texture with

glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);

which is OpenGL's default behaviour anyway.
By sampling outside the texture, this makes it impossible to support Nuklear's subimage feature, without rebaking the texture. I am depending on settings outside of Nuklear (OpenGL's or DirectX's texture repeat) and am not sure if I'm thus breaking Nuklear's philosophy of being backend agnostic. If yes, then this PR should propably stay unmerged. I'm unsure, feedback very welcome here.

Now there is the bool nk_style_window.tiled_background to switch between stretched and tiled.
I have thought about whether it's worth breaking the API by renaming struct nk_style_item fixed_background to struct nk_style_item image or whether to introduce a second struct nk_style_item tiled_background. But that just seemed weird, how you would be able to assign 2 images to the same nk_style_window, even though only one is shown. Not user-friendly IMO.

So I settled on renaming, breaking the API and introducing a bool switch instead. Maybe this should even be an enum to support more than just streched and tiled? Any feedback to my API approach is very welcome, as this doesn't feel worthy of a 5.0.0 version bump.

Moreover, for the tiling to work, Nuklear needs to know the image dimensions. This has not been the default so far, as I found out. This needs to change IMO, to better support styling image elements. For instance, nk_button_image still needs to be sized correctly with hacky layouting, because it stretches the image across the button, totally ignoring its aspect ratio. Another PR should tackle this. Also, simply setting nk_image.w and nk_image.h causes Nuklear to misinterpret images as subimages. nk_image_is_subimage() just checks img->w == 0 && img->h == 0, which breaks image drawing if Nuklear is provided with image dimensions, but not a subimage region. So the function has been changed to specifically check for !(img->region[0] == 0 && img->region[1] == 0 && img->region[2] == 0 && img->region[3] == 0); instead.

Finally, it's weird how we use nk_rect for all box defining stuff, but nk_image uses an ushort array to specify the sub image, maybe another TODO as well.

@dumblob
Copy link
Member

dumblob commented Apr 11, 2022

Ok, I've given this a quick thought and I think this really demonstrates many reasons why the simplistic solution of "always stretch" was chosen.

I'm really unsure we should land this change. The code itself is straightforward but the API doesn't feel right to me.

Some of the issues raised have a workaround - e.g. "assemble" a big bitmap from tiles manually and then use the resulting image as background. This can probably be even tweaked to react to resize events to maintain aspect ratio (won't be perfect but it'll get the job done IMHO).

Another problem is generally "what to do if I need to instruct backend during drawing" as Nuklear offers little to no API to do that. In other PRs we've discussed addition of new commands in "ad-hoc" fashion to facilitate the backend-specific needs in your apps. But so far I'm not aware of any finished efforts in this regard.

I think exactly as @FrostKiwi suggests above, we need to discuss these things first. Btw. if it's not an urgent matter @FrostKiwi , then there is also neonuklear 😉.

What are your thoughts @Hejsil , @RobLoach , and others?

@RobLoach
Copy link
Contributor

This seems like an important feature to have to enable more power around skinning 👍

@@ -1303,7 +1316,10 @@ nk_convert(struct nk_context *ctx, struct nk_buffer *cmds,
} break;
case NK_COMMAND_IMAGE: {
const struct nk_command_image *i = (const struct nk_command_image*)cmd;
nk_draw_list_add_image(&ctx->draw_list, i->img, nk_rect(i->x, i->y, i->w, i->h), i->col);
if(ctx->style.window.tiled_background)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested in isolation and didn't catch, that this line is garbage.
It forces all images to be treated as tiled and breaks styling.

Copy link
Contributor Author

@FrostKiwi FrostKiwi left a comment

Choose a reason for hiding this comment

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

Tested in isolation and didn't catch, that I am breaking default styling by accidently modifying all UVs.
Gotta fix that first.

@FrostKiwi
Copy link
Contributor Author

I stepped through the rendering loop and am a bit lost.
At

case NK_COMMAND_IMAGE: {
in the switch NK_COMMAND_IMAGE of nk_convert(), how do I know whether this nk_command_image came from a nk_window or not?
I don't want to ham fist a "is_window" boolean into nk_command_image, so can I somehow use ctx->draw_list to know whether what I am processing is a nk_window or not?

@FrostKiwi
Copy link
Contributor Author

Introduced a new nk_command_type NK_COMMAND_IMAGE_TILED to deal with the difference. nk_panel_begin() deals with differentiating between window or not. I duplicated nk_draw_image to do that. The idea behind that is to get 9slice working as well going forward.

Supporting 9 slice with tiling has a use-case. The design guideline, that I'm hamfisting into Nuklear, has a noise texture + rounded corners for windows. As long as the tiled texture is noise, and only the center of the 9slice is tiled, then this works to create a nice visual style without seams and without the need to custom-size window style image backgrounds.

It's still kinda user-unfriendly having to specify the image sizes though. This needs documentation. Another TODO.

@RobLoach RobLoach added the 5.x This would be made for Nuklear 5.x label Apr 20, 2022
@FrostKiwi
Copy link
Contributor Author

Obsoleted by PR #466

@FrostKiwi FrostKiwi closed this Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.x This would be made for Nuklear 5.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants