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

[Fix] Quads of different background types not ordered #1873

Merged
merged 9 commits into from
May 29, 2023

Conversation

bungoboingo
Copy link
Contributor

@bungoboingo bungoboingo commented May 25, 2023

So caught up in the elation of lerping, I completely forgot to order quads of different background types 🤦

This will fix any issues with using quads of different background types on top of one another.

I made a separate order field of the layer::Quads struct to consolidate quads for rendering, similar to how we're doing it with triangle meshes already.

@hecrj
Copy link
Member

hecrj commented May 25, 2023

I have some ideas to improve layering overall in the backlog.

For now, having gradients always be on top of solid seems consistent with the way other primitives work for a specific layer. I don't think we should introduce ordering (primitive layering) at a layer level.

@hecrj hecrj added improvement An internal improvement rendering labels May 25, 2023
@hecrj hecrj added this to the 0.10.0 milestone May 25, 2023
@bungoboingo
Copy link
Contributor Author

bungoboingo commented May 25, 2023

This is just for grouping quads together to be rendered actually in order of when they're supposed to be. Right now you can't put a container with a solid background over a container with a gradient background or else the solid will be rendered over since we're rendering all solids & then all gradients. I'm not sure where else to put ordering besides at the layer level since that's where they're being grouped, unless we want to introduce like a depth value or something to primitives.

Current master:
Screenshot 2023-05-25 at 2 14 49 PM

Fixed:
Screenshot 2023-05-25 at 2 13 54 PM

fn view(&self) -> Element<'_, Self::Message, Renderer<Self::Theme>> {
        let small_center_box = container(":-)")
            .center_x().center_y()
            .height(100)
            .width(100)
            .style(theme::Container::Custom(Box::new(SolidBackgroundStyle)));

        let center_box = container(small_center_box)
            .center_x()
            .center_y()
            .height(300)
            .width(300)
            .style(theme::Container::Custom(Box::new(SolidBackgroundStyle)));

        container(center_box)
            .width(Length::Fill)
            .height(Length::Fill)
            .center_x()
            .center_y()
            .style(theme::Container::Custom(Box::new(GradientBackgroundStyle)))
            .into()
    }

@hecrj
Copy link
Member

hecrj commented May 25, 2023

Yeah, I know. This is layering at the primitive level. Just pointing out that it shouldn't be a problem soon.

But I forgot the fact that gradients can be used as backgrounds now... So we should definitely fix this before that exploration lands.

I guess I'm not happy with how the implementation details of the quad module are leaking into our layering logic. Any kind of index logic should be opaque.

We should move the Quads struct to the quad module, rename it to Layer, make it completely opaque, and expose methods that the layer module can use. That way we keep all the limitations of the quad pipeline in the quad module, and we can guarantee that the indices are only generated in a single place.

@bungoboingo
Copy link
Contributor Author

Adjusted in 10a68ac 👍

@hecrj hecrj added the bug Something isn't working label May 29, 2023
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Awesome!

Moved some code around to avoid the quad pipeline details from leaking into layer by removing layer::quad altogether! We should probably do the same thing with the other layer submodules; but no need to do that here.

Also simplified some minor things! Hopefully I didn't break anything.

Let's merge! 🚢

@hecrj hecrj enabled auto-merge May 29, 2023 23:21
@hecrj hecrj disabled auto-merge May 29, 2023 23:27
@hecrj
Copy link
Member

hecrj commented May 29, 2023

Found the right way to keep solid and gradient details also private!

@hecrj hecrj enabled auto-merge May 29, 2023 23:36
@hecrj hecrj merged commit 9253f76 into iced-rs:master May 29, 2023
@bungoboingo
Copy link
Contributor Author

Looks good, we can move the other layer submodule code into its respective pipeline implementation files later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working improvement An internal improvement rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants