fix: Take scale into account #1906

jtmcdole · 2022-09-14T01:55:31Z

Description

Take scale into account when calculating offsets
Memoize the work of offsets - don't calculate it each time.
Tests - everyone loves tests.

In scaled destination tile size, this would render the pyramids in different locations:

Example pre-fix wrongness:

Checklist

The title of my PR starts with a [Conventional Commit] prefix (fix:, feat:, docs: etc).
I have read the [Contributor Guide] and followed the process outlined for submitting PRs.
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.

Breaking Change

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

Related Issues

Fixes #1904

spydon · 2022-09-15T08:38:01Z

@kurtome @ufrshubham could you review this PR too? :)

kurtome · 2022-09-15T13:44:25Z

LGTM, although can you double check that _applyParallaxOffset looks right, it takes camera.zoom into account so I'm not sure if additional change are needed there. Maybe not since it's all relative anyways...

jtmcdole · 2022-09-15T13:52:29Z

LGTM, although can you double check that _applyParallaxOffset looks right, it takes camera.zoom into account so I'm not sure if additional change are needed there. Maybe not since it's all relative anyways...

Sure thing. Probably needs a test as well.

jtmcdole · 2022-09-15T14:15:41Z

LGTM, although can you double check that _applyParallaxOffset looks right, it takes camera.zoom into account so I'm not sure if additional change are needed there. Maybe not since it's all relative anyways...

Sure thing. Probably needs a test as well.

So it doesn't look like it uses the destination tile size into account.. and I'm not sure it should. Yes, destination tile scale might seem like a cheap "scale" or "zoom", but it could just be sizing different maps to be the same for mixing. I don't know what the original intent was behind having it passed in versus reading it from the map.... and now that Tiled Component is positionable - it's also scalable.

That mixed with my unfamiliarity with parallax and I'm not sure.

kurtome · 2022-09-15T14:21:08Z

LGTM, although can you double check that _applyParallaxOffset looks right, it takes camera.zoom into account so I'm not sure if additional change are needed there. Maybe not since it's all relative anyways...

Sure thing. Probably needs a test as well.

So it doesn't look like it uses the destination tile size into account.. and I'm not sure it should. Yes, destination tile scale might seem like a cheap "scale" or "zoom", but it could just be sizing different maps to be the same for mixing. I don't know what the original intent was behind having it passed in versus reading it from the map.... and now that Tiled Component is positionable - it's also scalable.

That mixed with my unfamiliarity with parallax and I'm not sure.

My hunch is that it will work fine as is because parallax is really a function of solely the current Viewport/Camera relative to the 0,0 position of the map. And 0.5 parallax is going to do the same thing regardless of the tile size (I think)

ufrshubham · 2022-09-15T14:48:52Z

parallax is really a function of solely the current Viewport/Camera relative to the 0,0 position of the map

This is correct. Tile size won't matter for parallax. I'll try to play around with the changes just to be sure. But if I don't post anything within the next few days, consider a LGTM from my side.

ufrshubham

Just some structural comments. Overall the changes look solid!

ufrshubham · 2022-09-17T08:05:32Z

packages/flame_tiled/lib/src/renderable_tile_map.dart

+  late double offsetX = layer.offsetX * scaleX + (parent?.offsetX ?? 0);

-  double get opacity {
-    if (parent != null) {
-      return parent!.opacity * layer.opacity;
-    } else {
-      return layer.opacity;
-    }
-  }
+  late double offsetY = layer.offsetY * scaleY + (parent?.offsetY ?? 0);

-  double get parallaxX {
-    if (parent != null) {
-      return parent!.parallaxX * layer.parallaxX;
-    } else {
-      return layer.parallaxX;
-    }
-  }
+  late double opacity = layer.opacity * (parent?.opacity ?? 1);

-  double get parallaxY {
-    if (parent != null) {
-      return parent!.parallaxY * layer.parallaxY;
-    } else {
-      return layer.parallaxY;
-    }
-  }
+  late double parallaxX = layer.parallaxX * (parent?.parallaxX ?? 1);
+
+  late double parallaxY = layer.parallaxY * (parent?.parallaxY ?? 1);


Any specific reason to not have these as getters?

Which ones? Offset, Opacity, or Parallax?

Main reason: performance. As getters, you're calculating this every single render loop. As late-calculations, they are calculated once and cached.

As per our discussion on discord, this is few as these values never change.

ufrshubham · 2022-09-17T08:29:28Z

packages/flame_tiled/lib/src/renderable_tile_map.dart

+  final Vector2 destTileSize;
+  final TiledMap map;


Is it possible to not have these in here? It look like it is unnecessarily increasing all the derived classes as well. For example, _UnrenderableLayer is not even using them.

Yes, I think it's possible. Let work on it.

Actually; they are needed for the scale and offset calculations. Tile, Image, and Group uses these. _UnrenderableLayer is just unlucky. RenderableTiledMap is allocated after the layers are created... maybe we just update the layers to point to RenderableTiledMap as the parent map?

Got idea. Also, unrenderable layer is basically the object layer and technically it can contain renderable tile objects. So this change is actually useful for adding rendering support for the object layer in future.

ufrshubham · 2022-09-17T08:32:41Z

packages/flame_tiled/lib/src/renderable_tile_map.dart

+  void handleResize(Vector2 canvasSize);

-  void refreshCache() {}
+  void refreshCache();


Maybe better to leave these with a default implementation. Will avoid some empty overrides in derived classes.

_RenderableLayer is abstract. Having these as default implementations actual lead to the bug that this is fixing.

- Take scale into account when calculating offsets - Memoize the work of offsets - don't calculate it each time. - Tests - everyone loves tests. Fixes flame-engine#1904

ufrshubham · 2022-09-18T04:17:06Z

These changes LGTM 🔥

spydon approved these changes Sep 14, 2022

View reviewed changes

spydon requested a review from a team September 14, 2022 13:37

jtmcdole force-pushed the fixTiledOffsetWhenScaled branch from 9557029 to a827fc7 Compare September 15, 2022 14:05

ufrshubham reviewed Sep 17, 2022

View reviewed changes

Take scale into account

6f53e78

- Take scale into account when calculating offsets - Memoize the work of offsets - don't calculate it each time. - Tests - everyone loves tests. Fixes flame-engine#1904

jtmcdole force-pushed the fixTiledOffsetWhenScaled branch from a827fc7 to 6f53e78 Compare September 17, 2022 23:18

ufrshubham mentioned this pull request Sep 18, 2022

Refactor packages/flame_tiled/lib/src/renderable_tile_map.dart #1913

Closed

spydon merged commit 27ab12f into flame-engine:main Sep 18, 2022

jtmcdole deleted the fixTiledOffsetWhenScaled branch September 18, 2022 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Take scale into account #1906

fix: Take scale into account #1906

jtmcdole commented Sep 14, 2022

spydon commented Sep 15, 2022

kurtome commented Sep 15, 2022

jtmcdole commented Sep 15, 2022

jtmcdole commented Sep 15, 2022

kurtome commented Sep 15, 2022

ufrshubham commented Sep 15, 2022

ufrshubham left a comment

ufrshubham Sep 17, 2022

jtmcdole Sep 17, 2022

ufrshubham Sep 18, 2022

ufrshubham Sep 17, 2022

jtmcdole Sep 17, 2022

jtmcdole Sep 17, 2022

ufrshubham Sep 18, 2022

ufrshubham Sep 17, 2022

jtmcdole Sep 17, 2022

ufrshubham commented Sep 18, 2022

fix: Take scale into account #1906

fix: Take scale into account #1906

Conversation

jtmcdole commented Sep 14, 2022

Description

Checklist

Breaking Change

Related Issues

spydon commented Sep 15, 2022

kurtome commented Sep 15, 2022

jtmcdole commented Sep 15, 2022

jtmcdole commented Sep 15, 2022

kurtome commented Sep 15, 2022

ufrshubham commented Sep 15, 2022

ufrshubham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ufrshubham commented Sep 18, 2022