Skip to content

Conversation

TheMaverickProgrammer
Copy link
Contributor

@TheMaverickProgrammer TheMaverickProgrammer commented Jul 23, 2025

Description

Primarily, this PR allows a creator to add components to a Tiled Component's RenderableLayer. Since these layers now draw their child component's after they draw their tile map data to the canvas, the outcome is that paintable components (such as SpriteComponent) are drawn in the order of the layers which enables foregrounds and other detail layers to visually behave as expected.

Second, the parallax effect and infinite tiling of layers is optimized. Each layer only incurs at most one canvas translate.

Last but not least, the map renders 1:1 with Tiled even with parallax effects. Flame is now WYSIWYG!

✏️ Note that this is not a breaking change!

PR Demonstration

Video of the flame_tiled example application with changes.
https://github.com/user-attachments/assets/bfa09575-be7b-4ae3-99af-96336ad44f67

Video of components drawing as expected in a custom Flame game.
https://github.com/user-attachments/assets/7a1b3bc7-7365-4a2d-81cc-7f23b6e12fc4

Here's an example of adding the player to a layer beneath the "foreground" layer.
Note that the water layer is also on top of the foreground layer and both layers are on top of the player as expected.
image

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

Closes #3594

@TheMaverickProgrammer TheMaverickProgrammer changed the title feat: [flame_tiled] Layers respect draw-order of added Components feat [flame_tiled] Layers respect draw-order of added Components Jul 26, 2025
@TheMaverickProgrammer TheMaverickProgrammer changed the title feat [flame_tiled] Layers respect draw-order of added Components feat: [flame_tiled] Layers respect draw-order of added Components Jul 26, 2025
@spydon
Copy link
Member

spydon commented Aug 11, 2025

@TheMaverickProgrammer This PR needs a little conflict resolving with main, but it should be quite trivial, it's mostly formatting.
Ping us when this is ready for review, we're planning to make a release in a few days if you think it'll be ready until then. :)

@TheMaverickProgrammer
Copy link
Contributor Author

@TheMaverickProgrammer This PR needs a little conflict resolving with main, but it should be quite trivial, it's mostly formatting. Ping us when this is ready for review, we're planning to make a release in a few days if you think it'll be ready until then. :)

Will do, and I'll be done with this soon.

@TheMaverickProgrammer
Copy link
Contributor Author

@spydon There's only two tests failing by a margin: the orthographic map with parallax effect and the shifted isometric map test.

The ortho map has a questionable golden test. If I open up the map in Tiled and center the viewport on (0,0) it looks different from both of our results. I compared my changes on another Flame project and it looks stable, so I think we may just need to upgrade the golden test.

The shifted isomorphic map has me reviewing the code many times over. I'm unsure what I missed. It's just the group is shifting too much. So some extra offset is happening somewhere. What's odd is that all other tests pass without issue. Maybe if you do a quick review you'll catch it before I do. I'll try to be done with this tomorrow.

@spydon
Copy link
Member

spydon commented Aug 14, 2025

The ortho map has a questionable golden test

Can you see what is wrong in the test? Golden tests on Flutter differ slightly on different OS:es, but I guess the diff is bigger than just a pixel off?

I'll see if I have time to review it today, and I'll add some other reviewers that are more familiar with this code.

@spydon spydon requested a review from ufrshubham August 14, 2025 06:31
@spydon
Copy link
Member

spydon commented Aug 14, 2025

@kurtome @ufrshubham @jtmcdole could you maybe review this PR if you have time? Since you are much more familiar with the flame_tiled code than I am. 😇

Copy link
Contributor

@kurtome kurtome left a comment

Choose a reason for hiding this comment

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

If I remember correctly the original implementation combined the layers into a single image, which was a performance optimization. So, I assume this CL gets rid of that optimization?

Personally I'm fine with that, I always try to keep my Tiled maps small to avoid performance issues.

The code changes look reasonable to me on a quick review. I could try to test this on my game later this week if that's helpful.

@TheMaverickProgrammer
Copy link
Contributor Author

The ortho map has a questionable golden test

Can you see what is wrong in the test? Golden tests on Flutter differ slightly on different OS:es, but I guess the diff is bigger than just a pixel off?

I'll see if I have time to review it today, and I'll add some other reviewers that are more familiar with this code.

Yes for the test_shifted.tmx particularly, the group offset seems to be skipped which is odd considering the components add their offset from their parent's. All others are a tiny percentile off due to pixel artifacts which can be patched by upgrading the golden test since layers now render differently.

@TheMaverickProgrammer
Copy link
Contributor Author

TheMaverickProgrammer commented Aug 14, 2025

I found and patched the issue for the test_shifted.tmx. However it revealed I need to re-crunch the numbers for the parallax behavior optimization. Because I'm accumulating the total offset for the optimization, I do need to track that, but also each layer is already potentially translating the canvas, we don't want to apply the accumulate offset again during that step. However the canvas translation does require that we take into consideration the parallax offset which does accumulate.

So it's a bit of a mess but solvable now that I see the mistake. My next push ought to be the final.

EDIT: The math revealed we can translate the canvas for each layer with local parallax values as long as our local parallax values takes into account their parent's as well. Writing up a fix.

…g partial parallax locality. This is equivalent to the product of all parent parallax coefficients.
@jtmcdole
Copy link
Contributor

If I remember correctly the original implementation combined the layers into a single image, which was a performance optimization. So, I assume this CL gets rid of that optimization?

Personally I'm fine with that, I always try to keep my Tiled maps small to avoid performance issues.

I don't think that's correct; TiledAtlas was added to batch rendering, but I'm pretty sure that was done on a per-layer. I'm not sure if I was clipping the batch draw to the camera (which would have been a good idea), and I wanted to add some y-order optimizations so external components could be rendered between the layers. It sounds like this PR is aiming to fix the later.

The code changes look reasonable to me on a quick review. I could try to test this on my game later this week if that's helpful.

Its a bit large of a PR for me to review while sipping my morning coffee, for sure. I don't see any tests showing "Foreground tiles obscure sprites." - or I am I missing some golden images?

@TheMaverickProgrammer TheMaverickProgrammer changed the title feat: [flame_tiled] Layers respect draw-order of added Components feat: [flame_tiled] Components added to Layers draw in order. Parallax is now WYSIWYG. Aug 16, 2025
@TheMaverickProgrammer TheMaverickProgrammer marked this pull request as ready for review August 16, 2025 03:55
@TheMaverickProgrammer
Copy link
Contributor Author

If I remember correctly the original implementation combined the layers into a single image, which was a performance optimization. So, I assume this CL gets rid of that optimization?
Personally I'm fine with that, I always try to keep my Tiled maps small to avoid performance issues.

I don't think that's correct; TiledAtlas was added to batch rendering, but I'm pretty sure that was done on a per-layer. I'm not sure if I was clipping the batch draw to the camera (which would have been a good idea), and I wanted to add some y-order optimizations so external components could be rendered between the layers. It sounds like this PR is aiming to fix the later.

The code changes look reasonable to me on a quick review. I could try to test this on my game later this week if that's helpful.

Its a bit large of a PR for me to review while sipping my morning coffee, for sure. I don't see any tests showing "Foreground tiles obscure sprites." - or I am I missing some golden images?

Good point. I can add a test for this feature. Do I also have permission to update the orthogonal golden?

@spydon
Copy link
Member

spydon commented Aug 18, 2025

Do I also have permission to update the orthogonal golden?

Go for it. :)
When possible, create new golden tests instead though.

@TheMaverickProgrammer
Copy link
Contributor Author

Moving this back into DRAFT status until I find a reasonable solution for parallax tests which expect visible camera offset.

@TheMaverickProgrammer TheMaverickProgrammer marked this pull request as draft September 16, 2025 03:06
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.

4 participants