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

SVGResource constructor has absolute size options #5776

Merged
merged 9 commits into from
Jun 24, 2019

Conversation

tobyspark
Copy link
Contributor

Description of change

SVG assets are resolution independent. At present, there is no way to precisely set the resolution they rasterise at during initialisation. Subsequent resizing will scale this fixed resolution asset – leading to blurriness etc.

While a scale multiplier is provided as a constructor option to alter the fixed render size, this cannot be used to set an exact size as the SVG size is unknown before instantiation. This PR provides alternative constructor options to set size directly.

Pre-Merge Checklist
  • Tests and/or benchmarks are included
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

PIXI.Sprite.from('foo.svg', {resourceOptions: {width: 42, height: 42}})
@tobyspark
Copy link
Contributor Author

Fixes underlying need of #5754

@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Jun 13, 2019

Need our SVG specialist here :) @JetLua yo!

I think there might be something wrong with that round , give me some time to think about it.

In general, your idea looks good.

@tobyspark
Copy link
Contributor Author

The round was already in the code. If I were coding this from scratch, it would have been a ceil, as the canvas should contain whatever is drawn without clipping?

@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Jun 13, 2019

Yes, but that also means that we need to use scale, not just width/height because we adjust aspect ratio slightly. Also just ceil isnt that good, Need eps too, like here: https://github.com/pixijs/pixi.js/blob/dev/packages/math/src/shapes/Rectangle.js#L216

Also something needs to be done about resolution. How do we pass it to BaseTexture? Do we pass it to baseTexture only, not to resource?

I can approve current version, but if you have time to think about round. resolution and ceil - that'll be even better! I can look at it in weekend too.

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.

OK, I approve, I know what kind of tests to do about rounding, and its better to be handled in separate PR.

@tobyspark
Copy link
Contributor Author

tobyspark commented Jun 13, 2019

On resolution – I did wonder about that. As a general design pattern, could passing in an optional rendering destination allow the particular from method to act appropriately? Here it could be to set the rasterisation to e.g. retina, but perhaps there are other optimisations. Though that sounds increasingly like a separate PR.

@JetLua
Copy link
Contributor

JetLua commented Jun 13, 2019

Need our SVG specialist here :) @JetLua yo!

Ah,thanks.

@englercj englercj changed the title SVRResource constructor has absolute size options SVGResource constructor has absolute size options Jun 13, 2019
Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

One small change, could you just make a small documentation change to this: http://pixijs.download/dev/docs/PIXI.resources.html#.autoDetectResource

Notice that width and height say BufferResource. Would be good to mention the overrides for SVG width/height.

packages/core/src/textures/resources/SVGResource.js Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jun 22, 2019

Codecov Report

Merging #5776 into dev will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #5776      +/-   ##
==========================================
+ Coverage   70.55%   70.57%   +0.01%     
==========================================
  Files         205      205              
  Lines       10389    10396       +7     
==========================================
+ Hits         7330     7337       +7     
  Misses       3059     3059
Impacted Files Coverage Δ
.../core/src/textures/resources/autoDetectResource.js 100% <ø> (ø) ⬆️
...ackages/core/src/textures/resources/SVGResource.js 90.47% <100%> (+1.19%) ⬆️

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 dd9c990...421fb71. Read the comment docs.

@bigtimebuddy bigtimebuddy added this to the v5.1.0 milestone Jun 22, 2019
tobyspark and others added 2 commits June 24, 2019 13:47
Co-Authored-By: bigtimebuddy <mattkarl@netflix.com>
Co-Authored-By: bigtimebuddy <mattkarl@netflix.com>
@tobyspark
Copy link
Contributor Author

@bigtimebuddy Good oversight. I've pushed two commits to address your points. As part of the documentation commit, I also changed the language from 'render' to 'rasterize'. More precise, and I'd say helpful to separate out from 'render' as in the ongoing rendering of the scene.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Nice thanks for making those fixes.

@bigtimebuddy bigtimebuddy merged commit ccdfa1e into pixijs:dev Jun 24, 2019
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.

5 participants