search

bytestring-net / bevy_lunex

Blazingly fast path based retained layout engine for Bevy entities, built around vanilla Bevy ECS.
https://bytestring-net.github.io/bevy_lunex/
Apache License 2.0
512 stars 21 forks source link

General feedback #6

Closed UkoeHB closed 10 months ago

UkoeHB commented 1 year ago

Comments and suggestions I had while reading/reviewing/using the crate.

Further thoughts I will drop in the comments of this issue. Overall I am excited to continue using this crate and to see it grow.

IDEDARY commented 1 year ago

Thanks for the feedback. I value these nitpicks very highly. My goal is to make it very simple and clear, but because I am spearheading this solo, I don't have that much time to focus on UX improvements, so thanks a lot for writing it all in one spot! I will jump on them as soon as I can!

Rename: is_within() -> contains_position() (or just contains()).

Great proposal, should make it more clear and defines the purpose better.

Rename (examples): system -> ui.

Yes, I am in middle of doing that, but i haven't done all of them, so it's inconsistent now.

Optimize: cursor_update() move gets outside loop.

I will take a look at it with the other proposal you did.

Why is UiTree a component and not a resource if you can only have one of them?

The limit is only temporary, I though being able to have multiple of them would be better, so you can split your UI stuff to different "sections" like HUD, MENU, etc. But the functionality is currently not there yet. I am working towards it though. I still have to figure out which window dimensions to pull to which UiTree.

The noun-first naming pattern is hard to read. For example: position_get() would be much better as get_position()

It might, I will consider it. I will take a look at naming of other libraries and decide how they do it.

instead of using pack(), let Widget::create() take impl Into, then call .into() on it.

I agree, I will get on that today.

In WindowLayout, relative is in units of % while _relative are normalized (inconsistent).

I am not sure what you mean. I am pretty sure everything with "relative" in it should be in %. Can you elaborate or point to specific cases?

In ImageElementBundle, the image_dimensions field is NOT intuitive. Increasing the numbers reduces the image size? Changing the ratio does nothing?

Then it needs better name or description. It should note that it doesn't represent the final rendered image size, but rather the source image size. By default it scales the image to fit in the widget and for that it needs the source image dimensions. So if the image is smaller, it needs to scale it more rather than if the image is super big, it needs to scale it down to fit the screen.

I'm glad you found my lib useful, I hope for it to grow better too.

UkoeHB commented 1 year ago

I am not sure what you mean. I am pretty sure everything with "relative" in it should be in %. Can you elaborate or point to specific cases?

Here is the test code I wrote to put an icon in the upper left corner:

    let icon = Widget::create(
            ui,
            &main_menu.end("icon"),
            WindowLayout{
                relative        : Vec2 { x: 5., y: 5. },  //5% of screen size away from upper left corner
                width_relative  : 0.10,  //10% of screen width
                height_relative : 0.10,  //10% of screen height
                ..Default::default()
            }.pack()
        ).unwrap();

As you can see, I need 5. for relative and 0.10 for _relative.

By default it scales the image to fit in the widget and for that it needs the source image dimensions.

I inserted a 500 x 500 png into my screen and image_dimensions needs to be Vec2::new(1.0, 1.0) in order to see the image. For a while I had it Vec2::new(500.0, 500.0) and the image wasn't visible because it shrank by a factor of 500... (very confusing bug). Maybe because I am using SolidLayout with SolidScale::Fit?

I am using v0.0.1 so maybe that's the problem (was just following the cyperpunk example). (ok no change after updating)

IDEDARY commented 1 year ago

Here is the test code I wrote to put an icon in the upper left corner: ... As you can see, I need 5. for relative and 0.10 for _relative.

Weird, it's not like this for me and I don't even remember that being a thing in the past (0.0.1). It really shouldn't do that. Are you sure main_menu is properly set up to scale to 100% of the window? Anyways, try using this to tinker around: gist It's an extract from examples that I am preparing right now. It uses the most up-to-date practices and functions.

Tell me if you are experiencing the same issues or not. Based on that we can rule out some things.

UkoeHB commented 1 year ago

Yeah my code is pretty simple, I don't see any real difference from your gist.

Lol ok, I had the numbers all mixed up. The relative scaling needs to be 20. and the image dimensions need to be 200.. Doing that gives me the right size image (it's actually a 200x200 image... whoops).

IDEDARY commented 1 year ago

I inserted a 500 x 500 png into my screen and image_dimensions needs to be Vec2::new(1.0, 1.0) in order to see the image. For a while I had it Vec2::new(500.0, 500.0) and the image wasn't visible because it shrank by a factor of 500... (very confusing bug). Maybe because I am using SolidLayout with SolidScale::Fit?

I added image element example here, in examples/ui/images

Lol ok, I had the numbers all mixed up. The relative scaling needs to be 20. and the image dimensions need to be 200.. Doing that gives me the right size image (it's actually a 200x200 image... whoops).

So... It works as expected now or is there still some confusion regarding that? :D

UkoeHB commented 1 year ago

So... It works as expected now or is there still some confusion regarding that? :D

Yeah it's working now, sorry about that.

IDEDARY commented 1 year ago

Good, no problem :) Also I finished some of the UX changes you proposed and published version 0.0.4 to crates.io. They are: