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

perf: Avoid Vector2 creation in Sprite.render #2261

Merged
merged 3 commits into from
Jan 10, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions packages/flame/lib/src/sprite.dart
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ class Sprite {
);
}

// Used to avoid the creation of new Vector2 objects in render.
late final _tmpRenderPosition = Vector2.zero();
late final _tmpRenderSize = Vector2.zero();
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably no need to make them late.

Copy link
Member Author

Choose a reason for hiding this comment

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

No harm in doing so right? If some user has some big collection of sprites that aren't being rendered yet they have a little bit less overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the overhead is that there needs to be a runtime check whether the late variable has actually been initialized or not. And I'm not sure whether such a check is debug-mode only, or if it is also done in production...

Copy link
Member Author

Choose a reason for hiding this comment

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

That must be very minimal? But I don't know how it is done.
I'd say it is worth the trade-off of not carrying around two Vector2 before they are used.
I can do some benchmarking tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another possibility is to declare them static. Then you have only one object instance no matter how many sprites

Copy link
Member Author

Choose a reason for hiding this comment

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

True, single thread ftw 😄

spydon marked this conversation as resolved.
Show resolved Hide resolved

/// Renders this sprite onto the [canvas].
///
/// * [position]: x,y coordinates where it will be drawn; default to origin.
Expand All @@ -97,18 +101,30 @@ class Sprite {
Anchor anchor = Anchor.topLeft,
Paint? overridePaint,
}) {
final drawPosition = position ?? Vector2.zero();
final drawSize = size ?? srcSize;

final delta = anchor.toVector2()..multiply(drawSize);
final drawRect = (drawPosition - delta).toPositionedRect(drawSize);
if (position != null) {
_tmpRenderPosition.setFrom(position);
} else {
_tmpRenderPosition.setZero();
}

if (size != null) {
_tmpRenderSize.setFrom(size);
} else {
_tmpRenderSize.setFrom(srcSize);
}
spydon marked this conversation as resolved.
Show resolved Hide resolved

_tmpRenderPosition.setValues(
_tmpRenderPosition.x - (anchor.x * _tmpRenderSize.x),
_tmpRenderPosition.y - (anchor.y * _tmpRenderSize.y),
);

final drawRect = _tmpRenderPosition.toPositionedRect(_tmpRenderSize);
final drawPaint = overridePaint ?? paint;

canvas.drawImageRect(image, src, drawRect, drawPaint);
}

/// Return a new Image based on the [src] of the Sprite.
/// Return a new [Image] based on the [src] of the Sprite.
///
/// **Note:** This is a heavy async operation and should not be called inside
/// the game loop. Remember to call dispose on the [Image] object once you
Expand Down