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

New image handling and image types #466

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

FrostKiwi
Copy link
Contributor

@FrostKiwi FrostKiwi commented Jun 12, 2022

So far Nuklear has ignored image aspect ratio and getting images to properly display has been by hacking your way around the layout system. This PR introduces enum nk_image_type in struct nk_image to remedy this and obsoletes PR #444. The new image types are demonstrated here:

image_types.mp4

The image type is a setting defined upon image creation and does not need to be supplied each time nk_image is called. The API for displaying the image stays the same. nk_image() generates the NK_COMMAND_IMAGE with correct sizes to honor aspect ratio based on the surround layout.

  • NK_IMAGE_STRETCH: The default stretching, as required for styling to function properly
  • NK_IMAGE_FILL: Honors the image's aspect ratio, fills out the entire target rect
  • NK_IMAGE_FIT: Honors the image's aspect ratio, fits the entire image into the target rect
  • NK_IMAGE_CENTER: 1:1 pixel-mapping, center the image
  • NK_IMAGE_TILE: 1:1 pixel-mapping, sample image outside of UV range. It is required that the backend is set to texture tiling mode. (Which is the default OpenGL Texture behaviour anyways.). This will not work with nk_subimage in this PR's current form, as subimage just modifies UVs. This really has to be addressed, but it cannot happen inside of the backend agnostic nuklear.h. I think providing a utility function in nuklear_util.c to copy a the subimage rect into a new buffer, so the user may upload that buffer to create a new texture.

This PR is an API break: It requires the user to provide image-size upon image creation, no exceptions. Also the image type has to be provided upon nk_image creation. This is required for any of this to work and is a long overdue change IMO. This PR is also essential for the interface element that I'm porting to Nuklear "cards" to work. Since this API break is needed, I took the chance to rename some functions for more clarity. Eg. struct nk_image nk_image_ptr(void*); is now nk_image_from_ptr(void*, nk_ushort w, nk_ushort h, enum nk_image_type);, to make it clear, that this function is for generating a new nk_image from a pointer, not for providing the pointer of an nk_image.

Now there is no more new command type needed, as I did in PR #444, which is much nicer to maintain.

TODO:

  • Fix demo files to speak the new API, so the Github's build pipline does not complain
  • Make nk_subimage versions of all the new image types, except NK_IMAGE_TILE
  • Introduce a function in 'nuklear_util.c' to solve the problem of nk_subimage being incompatible with tiled image drawing. (Unless someone has a better idea...)

nuklear.h Outdated Show resolved Hide resolved
nuklear.h Outdated Show resolved Hide resolved
@dumblob
Copy link
Member

dumblob commented Jun 12, 2022

Generally this looks like a promising approach to me.

It requires the user to provide image-size upon image creation, no exceptions.

This though is a bit too strict in my opinion. I can not imagine myself tracking dimensions of each image, icon, ... I use in my app.

Why actually not emulate the current "old" API (which was extremely limited but did not require specifying dimensions) using this your new API?

Actually I dare to think this new approach could be implemented in a backwards compatible way (i.e. just extending the current API) without the need to introduce 5.x.y version.

What do you all think?

@FrostKiwi
Copy link
Contributor Author

What do you all think?

Huh... Now that you mention it, maybe that is too strict. So keep the old commands, always defaulting to NK_IMAGE_STRETCH and create a set of new ones, where you have to specify size I guess?

I can not imagine myself tracking dimensions of each image, icon

Isn't that always automatically known when you load the image?

@dumblob
Copy link
Member

dumblob commented Jun 12, 2022

Huh... Now that you mention it, maybe that is too strict. So keep the old commands, always defaulting to NK_IMAGE_STRETCH and create a set of new ones, where you have to specify size I guess?

Yes, right now that would be my preference. But I have a weird feeling I am missing something so please come up with other ideas if you have any 😉.

I can not imagine myself tracking dimensions of each image, icon

Isn't that always automatically known when you load the image?

Yes. What I meant though is the syntactical pollution of maintaining variables (scalars, arrays, maps, whatever...) holding the dimensions for no apparent benefit. Does that make more sense now?

src/nuklear_image.c Outdated Show resolved Hide resolved
@FrostKiwi FrostKiwi changed the title [5.x] New image handling and image types New image handling and image types Jun 14, 2022
@FrostKiwi
Copy link
Contributor Author

Yes, right now that would be my preference. But I have a weird feeling I am missing something so please come up with other ideas if you have any 😉.

Created two sets of commands, one with type in the name, reverted changes to old commands, removed [5.x] tag from issue titled, since there is no more API break.

@dumblob
Copy link
Member

dumblob commented Jun 17, 2022

Nice, thanks. Do you still plan to work on the last TODO items in the initial post above or shall we make our hands dirty with a full-scale review already?

@RobLoach
Copy link
Contributor

RobLoach commented Jul 9, 2022

I really like where this is going!

@FrostKiwi
Copy link
Contributor Author

Note to self: Use Nuklear's internal floor(), instead of the math.h floor(). Creates an unresolved dependancy

@wuxxerno
Copy link

Is there anything I can help with to get this merged?

@FrostKiwi
Copy link
Contributor Author

Is there anything I can help with to get this merged?

I've given you access to my repo and the branch, that this PR is connected to. Go wild, in case you have any ideas.

I'll revive my Nuklear efforts over the weekend. What it got stuck on last time I wanted to contribute, are the Krita design files + nk_card code, that I couldn't publish due to being derived from company internal design guideslines.

I'll recreate them over the weekend from FOSS material, post that and then return to this PR.

@wuxxerno
Copy link

I was able to start on this today. I updated the NK_IMAGE_CENTER case to use uv coords instead as it didnt work correctly on dynamic layout. I will continue with implementing the nk subimage as per @FrostKiwi todo notes.

@FrostKiwi
Copy link
Contributor Author

I was able to start on this today. I updated the NK_IMAGE_CENTER case to use uv coords instead as it didnt work correctly on dynamic layout. I will continue with implementing the nk subimage as per @FrostKiwi todo notes.

Many many thanks for that excellent support. Finished my Pull Request regarding the skin design file #548 and will return to look at Nuklear next week.

@FrostKiwi
Copy link
Contributor Author

FrostKiwi commented Mar 1, 2023

as it didnt work with dynamic layouting

@wuxxerno Ohh wow, you were right. How did I miss that. Glad to have this figured out, I really didn't want to do the math there to get it to work without scissor^^

You made changes directly to nuklear.h, instead of the src/ source code of nuklear. That huge nuklear.h is auto-generated and should not be changed. You should do your changes in src/ and either include directly from the source or build the final nuklear.h with src/paq.sh or src/paq.bat on windows.
For instance I test in demo/glfw_opengl3, where I have a test.sh, which rebuilds the final nuklear.h and compiles the test program.

cd ../../src && sh paq.sh && cd - && make && ./bin/demo.exe

I moved your contributions over to with fc97bb9 to src/nuklear_vertex.c

@wuxxerno
Copy link

wuxxerno commented Mar 1, 2023

as it didnt work with dynamic layouting

@wuxxerno Ohh wow, you were right. How did I miss that. Glad to have this figured out, I really didn't want to do the math there to get it to work without scissor^^

You made changes directly to nuklear.h, instead of the src/ source code of nuklear. That huge nuklear.h is auto-generated and should not be changed. You should do your changes in src/ and either include directly from the source or build the final nuklear.h with src/paq.sh or src/paq.bat on windows. For instance I test in demo/glfw_opengl3, where I have a test.sh, which rebuilds the final nuklear.h and compiles the test program.

cd ../../src && sh paq.sh && cd - && make && ./bin/demo.exe

I moved your contributions over to with fc97bb9 to src/nuklear_vertex.c

I see! thank you for explaining and correcting that.

@wuxxerno
Copy link

wuxxerno commented Mar 5, 2023

I just finished implementing the nk_subimage handling. (All but NK_IMAGE_TILE)
Im not really sure on how to proceed with problem of tiling subimages

I think providing a utility function in nuklear_util.c to copy a the subimage rect into a new buffer, so the user may upload that buffer to create a new texture.

Did you mean something totally generic like a function that swaps and crops two user inputted buffers that the user then can use upload as a new texture for a new nk_image? @FrostKiwi

@dumblob
Im happy to work on any feedback if you would like to start reviewing.

@FrostKiwi
Copy link
Contributor Author

FrostKiwi commented Mar 5, 2023

I just finished implementing the nk_subimage handling. (All but NK_IMAGE_TILE)

That is HUGE! It was the quint essential challenge remaining to get this PR to an at least theoretically mergable state.

Im not really sure on how to proceed with problem of tiling subimages

In short: there is no solution from the position of a backend agnostic library, because the tile needs to tile (heh). Something only the backend can do. (Except rendering a ton of nk_images to tile window, resulting in horribly wasteful performance and blowing past all kinds of buffers on a high res screen with a small tile)

Did you mean something totally generic like a function that swaps and crops

Yes. A function which writes the content of an nk_subimage as defined by the subregion to a buffer, to a buffer specified by the user with a pointer. Similar to snprintf(), it returns the written size and if the user pointer is NULL, it returns just the needed size without writing anything, so the user may malloc() the needed amount.

The user may then setup tiling correctly. (By uploading as a separate texture, or bundle them in a texture array). It's less of a solution and more of a convenient tool to help the user setup tiling. (technically there is no reason this couldn't be done via a custom shader with frac() returning UVs in GLSL even without such an nk_util.h, but we should give at least some kind of universal solution, even if naive)

@malespiaut
Copy link

Hello,

I've tried to reproduce your (@FrostKiwi) example code using the SDL2 Surface backend, and the whole context crashes when trying to display NK_IMAGE_TILE.

record.mp4

Here's my modified nuklear/demo/sdl_renderer/main.c with the needed images.

sdl_renderer.zip

I hope this isn't a bug in my own implementation!

@FrostKiwi
Copy link
Contributor Author

FrostKiwi commented Apr 8, 2024

@malespiaut

Hello,

I've tried to reproduce your (@FrostKiwi) example code using the SDL2 Surface backend, and the whole context crashes when trying to display NK_IMAGE_TILE.

This should be really working. As mentioned in the doc, NK_IMAGE_TILE is a special case requiring cooperation from the rendering backend:

    /* Just draw the image with UVs for 1:1 pixel mapping. Image is draw outside
       it's bounds. What is drawn outside the bounds is determined by the
       backend. Eg. To get tiling behaviour with OpenGL, the texture should have
       been uploaded to the GPU with GL_REPEAT:
       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT);
       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);
       (Which is the default OpenGL Texture behaviour anyways.) */

I followed your example and implemented it in SDL_renderer, loaded up the texture via SDL_Texture *texture = IMG_LoadTexture(renderer, "bin/bunny_wide.png"); and let it run in NK_IMAGE_TILE mode.
Same deal, the rendering disappears. Neither you video, nor my recreation resulted in a crash though, just the blue background.

Checked SDL_GetError() and here is the cause:
[Error] Values of 'uv' out of bounds 0.525253 1.420455 at 320/360
So the cause is clear. No crash, no mishandling, SDL2 (When handling the textures through its API) just refuses to draw images outside its UV bounds, which is required for the most simple implementation of tiling.

I always used GLFW for my projects and never used SDL2, so from here on out I can't say why SDL2 refuses drawing a texture outside it's bounds. Here is what ChatGPT had to say on the topic:

SDL itself doesn't directly support repeating textures (tiling) by setting texture coordinates outside this range as you might in OpenGL or Direct3D. This is because SDL abstracts away the underlying graphics API to provide a simple, cross-platform rendering API, which doesn't expose direct control over texture wrap modes.

PS: gave you commit access to this Pull Request, in-case you want to contribute a demo or fix.

@malespiaut
Copy link

I followed your example and implemented it in SDL_renderer, loaded up the texture via SDL_Texture *texture = IMG_LoadTexture(renderer, "bin/bunny_wide.png"); and let it run in NK_IMAGE_TILE mode. Same deal, the rendering disappears. Neither you video, nor my recreation resulted in a crash though, just the blue background.

There is no crash. It's just me quitting the application and starting it again to show the bug in three different way (resize, scroll, different window).

Sorry for my miscommunication.

PS: gave you commit access to this Pull Request, in-case you want to contribute a demo or fix.

Thank you very much, I'll do that!

Fix minor compilation warnings in image handling
@RobLoach
Copy link
Contributor

RobLoach commented Jul 3, 2024

It has been officially more than 2 years since this Pull Request was opened. Is there anything we can do to help push it forwards? What else is missing?

@RobLoach RobLoach added the High Priority This would be great to get in. label Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority This would be great to get in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants