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

Convert Sprite to TypeScript #6375

Merged
merged 6 commits into from
Feb 3, 2020
Merged

Convert Sprite to TypeScript #6375

merged 6 commits into from
Feb 3, 2020

Conversation

Zyie
Copy link
Member

@Zyie Zyie commented Feb 1, 2020

Description of change

Converts the sprite package to TypeScript.

The main changes are:

  • Created TextureSource and SpriteSource types
  • Made _worldID in Transform be public
Pre-Merge Checklist
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

Copy link
Collaborator

@ivanpopelyshev ivanpopelyshev left a comment

Choose a reason for hiding this comment

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

Overall it looks good, we just need to confirm a few points

@@ -10,6 +10,8 @@ import { TextureMatrix } from './TextureMatrix';

const DEFAULT_UVS = new TextureUvs();

export type TextureSource = string|HTMLImageElement|HTMLCanvasElement|HTMLVideoElement|BaseTexture;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use ImageSource from @pixi/core here instead of HTMLxxx stuff

Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, we have to test if Sprite.from(resource) works too

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I've updated the type. I've not really written any tests, is that something you can help me with?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont mean to write test, just to check if it works in pixi-playground. I can do it tomorrow

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind, its part of our discussion about texture resource parameters.

packages/math/src/Transform.ts Show resolved Hide resolved
packages/sprite/src/Sprite.ts Outdated Show resolved Hide resolved
import { BLEND_MODES } from '@pixi/constants';
import { Container } from '@pixi/display';
import { Shader, Texture, Filter, Renderer, IBaseTextureOptions, TextureSource } from '@pixi/core';
Copy link
Collaborator

Choose a reason for hiding this comment

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

My hands just want to separate it to real import and import type but we cant do that before new TS is released, also our linter doesnt like two import lines from same module

@@ -271,18 +309,18 @@ export class Sprite extends Container
{
// if the sprite is trimmed and is not a tilingsprite then we need to add the extra
// space before transforming the sprite coords.
w1 = trim.x - (anchor._x * orig.width);
w1 = trim.x - (anchor.x * orig.width);
Copy link
Collaborator

Choose a reason for hiding this comment

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

See the problem? @finscn wont be happy that we removed his "optimizations". That's a kind of hotspot and it uses extra property calls. LEts confirm that @GoodBoyDigital is ok with that.

Anchor is always ObservablePoint so maybe we need to make _x and _y visible, its not the only place we use them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made _x and _y internal and reverted these changes

@bigtimebuddy bigtimebuddy added this to the v5.3.0 milestone Feb 1, 2020
@codecov-io
Copy link

codecov-io commented Feb 1, 2020

Codecov Report

Merging #6375 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #6375   +/-   ##
=======================================
  Coverage   74.09%   74.09%           
=======================================
  Files         104      104           
  Lines        5849     5849           
=======================================
  Hits         4334     4334           
  Misses       1515     1515

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2168c5d...21b8490. Read the comment docs.

@bigtimebuddy bigtimebuddy merged commit b11225e into dev Feb 3, 2020
@bigtimebuddy bigtimebuddy deleted the dev-sprite-typescript branch February 3, 2020 18:47
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.

None yet

4 participants