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 for bounding box of path, opacity of group, some transform #1671

Closed
wants to merge 5 commits into from
Closed

Fix for bounding box of path, opacity of group, some transform #1671

wants to merge 5 commits into from

Conversation

asturur
Copy link
Member

@asturur asturur commented Sep 16, 2014

This PR adds:
Path Class
Correct calculation of bounding box for paths ( with bezier and arc commands ).
Removed render method in path class to use the standard obejct one
Removed 1 ctx.transform in path _render method

Object Class
Correct rendering of opacity for groups, added setOpacity method.
Removed _Transform method, it was confusing in the render logic. "transform" method is enough
TransformMatrix on single objects are not supported yet, but behaves a little better, at least the object now follow mouse movements and doesn't go in different direction.

Text class:
Made some changes to adapt to object class.

arc.js
Add functions to calculate the bounding box.

Removed own render function , used the parent one.
Created new parse dimension, using the structure of render function to iterate the path.
Removed one translation from render cycle
Top and left is specified or calculated, while width and height are always calculated.
Removed globalalpha from transform method.
Added private setOpacity Method that checks recursively the groups both for groups and path-groups.
Commented "setup fill rule" to avoid confusion, it should be rename in "setupCompositeOperation".
removed _transform method
Object not grouped with transformMatrix are  still not supported but behaves lot better than before.
Some changes to follow object class changes.
Added getBoundsOfCurve and getBoundsOfArc functions
@asturur
Copy link
Member Author

asturur commented Sep 16, 2014

@kangax
I commented setupFillRule and restoreFillRule in object Class, there is some confusion.
Here we have fill-rule that is a property of path element.
This fill rule is used with the .fill method of the canvas. ( .fill() or .fill('even-odd') ).

We use fillRule even to store generic object property that gets applied to globalCompositeOperation, such as "nonzero", "destination-over" , "destination-atop" etc etc etc.

I think we should change both the name of the latter property and the name of the method in "compositeRule" and setCompositeRule , restoreCompositeRule to avoid confunsion.

If we do not make that change path class cannot have both a fillrule even-odd and a destination-over composite operaiton.

What do you think? can i make the change?

of course i will fix the unit testing if you approve all those changes.

@Kienz
Copy link
Collaborator

Kienz commented Sep 17, 2014

@asturur Can you split this PR into eg. 3 PR's?

  • Bounding box changes
  • Opacity changes
  • Text.js changes

@asturur
Copy link
Member Author

asturur commented Sep 17, 2014

i can split in opacity, bounding box, and general rendering changes.

but is a lot of work to rewrite, if you need it ok. let me know.

let me know for my question about globalcompositeoperation, otherwise i
cannot complete it
Il 17/set/2014 09:50 "Stefan Kienzle" notifications@github.com ha scritto:

@asturur https://github.com/asturur Can you split this PR into eg. 3
PR's?

  • Bounding box changes
  • Opacity changes
  • Text.js changes


Reply to this email directly or view it on GitHub
#1671 (comment).

@asturur
Copy link
Member Author

asturur commented Sep 17, 2014

Other problem is that i have to make a path bounding box patch, that when applied needs custom fix to don't break the render ( cause now left, top, width etc etc will change ), and those render changes are just made to make that pr works. Maybe the only one that i can extract is the opacity change.
This i will do know, so the rest we can discuss.

@asturur
Copy link
Member Author

asturur commented Sep 17, 2014

@kangax, @Kienz PR #1672 is just for opacity.

@asturur asturur closed this Sep 20, 2014
@asturur asturur deleted the Fix-for-bounding-box-of-path,-opacity-of-group,-some-transform branch September 20, 2014 09:26
@asturur asturur restored the Fix-for-bounding-box-of-path,-opacity-of-group,-some-transform branch September 20, 2014 09:26
@asturur asturur deleted the Fix-for-bounding-box-of-path,-opacity-of-group,-some-transform branch September 20, 2014 12:35
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

2 participants