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

Fix lineStyle+fillStyle drawCalls count #5981

Merged
merged 4 commits into from Aug 1, 2019
Merged

Conversation

ivanpopelyshev
Copy link
Collaborator

@ivanpopelyshev ivanpopelyshev commented Aug 1, 2019

lineStyle never goes into one drawCall with fillStyle if batching is turned off. That means PixiJS just spams drawCalls for graphics! Bug exists even in 5.0.1

image

Demo has extra feature: it shows wireframe for fills!

Bug, 2 drawcalls: https://www.pixiplayground.com/#/edit/oXRqFpfuva9wwP6B_0V1d

Fixed, 1 drawcall: https://www.pixiplayground.com/#/edit/AjVLQ2NLsXQXM9KrUWCcp

@ivanpopelyshev ivanpopelyshev added Plugin: Graphics 🔥 High Priority These are PRs or issues that are extremely important to address either because they are from sponsor labels Aug 1, 2019
@ivanpopelyshev ivanpopelyshev added this to the v5.1.1 milestone Aug 1, 2019
@bigtimebuddy
Copy link
Member

This doesn’t make sense to me. Native is a line specific property. It doesn’t make sense to throw it on Fill. Are you sure you’re fixing the right thing here?

@ivanpopelyshev
Copy link
Collaborator Author

ivanpopelyshev commented Aug 1, 2019

Oh, right, I didnt explain it, sorry :)

One big problem: https://github.com/pixijs/pixi.js/blob/dev/packages/graphics/src/GraphicsGeometry.js#L653
Not big, but adds extra batch: https://github.com/pixijs/pixi.js/blob/dev/packages/graphics/src/GraphicsGeometry.js#L484

false !== undefined.

@bigtimebuddy
Copy link
Member

So fix that case. If native is undefined use false. Don’t change the API to something illogical.

@ivanpopelyshev
Copy link
Collaborator Author

OK then, do you want me to change those to != and comment "do not touch that" ?

@bigtimebuddy
Copy link
Member

Just don't change LineStyle or FillStyle. It's not fixing the problem, it just creating API confusion.

For fills, can you just do !!style.native

@ivanpopelyshev
Copy link
Collaborator Author

ok, done

@codecov-io
Copy link

codecov-io commented Aug 1, 2019

Codecov Report

Merging #5981 into dev will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #5981   +/-   ##
=======================================
  Coverage   70.43%   70.43%           
=======================================
  Files         208      208           
  Lines       10516    10516           
=======================================
  Hits         7407     7407           
  Misses       3109     3109
Impacted Files Coverage Δ
packages/graphics/src/GraphicsGeometry.js 73.12% <50%> (ø) ⬆️

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 5a3593e...ffa2b7e. Read the comment docs.

@eXponenta
Copy link
Contributor

Thanks!
Now its works clean!
image

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.

Thanks, better!

Copy link
Member

@GoodBoyDigital GoodBoyDigital left a comment

Choose a reason for hiding this comment

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

coool 👍

@bigtimebuddy bigtimebuddy merged commit 35d0b92 into dev Aug 1, 2019
@bigtimebuddy bigtimebuddy deleted the fix-linestyle-drawcall branch August 1, 2019 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔥 High Priority These are PRs or issues that are extremely important to address either because they are from sponsor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants