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: draw portrait with canvas #32

Merged
merged 9 commits into from
May 27, 2024
Merged

Conversation

puroong
Copy link
Contributor

@puroong puroong commented May 25, 2024

Description

Resolves #14

  • store all portrait images in imageMap
    • this is necessary in order to load images with spriteImage
  • draw portrait with canvas
  • animate portrait with setInterval

Memory Usage

  • 1000 Portraits

with img tag (as is)

image

with canvas

Screenshot 2024-05-26 at 05 04 16

Comment on lines 71 to 73
if (animate) {
setInterval(animate, 1000 / keyFrameYs.length);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to return () => clearInterval(interval) so that it gets stopped properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the effect needs to support the paused flag and ensure the animation is not started.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


const { position } = unit.sprite.portrait;
const y = -(position.y + (variant || 0)) * PortraitHeight + 'px';
const alternateY = -(position.y + (variant || 0) + 6) * PortraitHeight + 'px';
const keyFrameYs = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can call this positions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(position.y + (variant || 0)) * PortraitHeight,
(position.y + (variant || 0) + 6) * PortraitHeight,
];
let curKeyFrameY = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this currentPosition, and move it inside the useLayoutEffect call. We can't mutate variables created in render like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

</div>
);
});

const vars = new CSSVariables<'x' | 'y' | 'alternate-y'>('p');

const portraitStyle = css`
contain: content;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this line since we aren't overflowing the element anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpojer
Copy link
Contributor

cpojer commented May 25, 2024

@puroong wow, thanks for the contribution! This is very nice, and will make Athena Crisis work much better on low-end devices. I left a few comments to make sure we are cleaning things up.

@puroong
Copy link
Contributor Author

puroong commented May 26, 2024

@cpojer
Thanks for the opportunity! I addressed the feedbacks and its ready for review 😄

@puroong
Copy link
Contributor Author

puroong commented May 26, 2024

My bad... I should have run the tests in local..
fixed lint errors in this commit!

style: lint

@cpojer cpojer merged commit 4eeddfc into nkzw-tech:main May 27, 2024
2 checks passed
@cpojer
Copy link
Contributor

cpojer commented May 27, 2024

Thank you for your contribution. The funds were distributed to you.

ad1992 pushed a commit to ad1992/athena-crisis that referenced this pull request May 30, 2024
…ch#18) (nkzw-tech#32)

Co-authored-by: Connor Lindsey <[email protected]>
GitOrigin-RevId: 80499df94c87243652a05c36fa5c43bd079fae6b
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.

[Performance] Reduce memory usage when rendering Portraits
2 participants