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

Nested group matrix should not be reset. #1711

Closed
sasensi opened this issue Sep 24, 2019 · 7 comments · Fixed by #1717
Closed

Nested group matrix should not be reset. #1711

sasensi opened this issue Sep 24, 2019 · 7 comments · Fixed by #1717
Labels
cat: matrix status: PR proposed an active pull request should fix this issue type: bug

Comments

@sasensi
Copy link
Contributor

sasensi commented Sep 24, 2019

Description/Steps to reproduce

Originally reported here.
When two groups are nested and the parent has matrix applied and the child has matrix not applied.
If we first do a transformation on the child, its matrix is updated as expected.
But if we then do a transformation on the parent, the child matrix is unexpectedly reset.

Link to reproduction test-case

Here is a sketch reproducing the issue.

// Create nested groups.
var childGroup = new Group({ applyMatrix: false });
var parentGroup = new Group([childGroup]);

// Transform child.
childGroup.scale(10);

// Matrix is updates as expected.
// Outputs: 10
console.log(childGroup.scaling.x);

// Transform parent.
parentGroup.scale(1.1);

// Matrix was unexpectedly reset.
// Outputs: 1 (instead of 10)
console.log(childGroup.scaling.x);

Expected result

Child matrix should not be reset.

sasensi added a commit to sasensi/paper.js that referenced this issue Oct 6, 2019
When a group had `applyMatrix` set to `false`, when its parent's matrix
was applied, its matrix was applied to its children then it was reset.
This makes sure that in this case, parent matrix is only added to child
matrix but not applied to child's children and that child's matrix is
not reset.
Closes paperjs#1711
@sasensi sasensi added the status: PR proposed an active pull request should fix this issue label Oct 6, 2019
lehni pushed a commit that referenced this issue Nov 5, 2019
When a group had `applyMatrix` set to `false`, when its parent's matrix
was applied, its matrix was applied to its children then it was reset.
This makes sure that in this case, parent matrix is only added to child
matrix but not applied to child's children and that child's matrix is
not reset.
Closes #1711
@lehni
Copy link
Member

lehni commented Dec 13, 2019

@sasensi merging this has actually broke the examples/Paperjs.org/PathIntersections.html example, see attached screenshot.

Could you look into what's going wrong here?

image

@lehni lehni reopened this Dec 13, 2019
@lehni
Copy link
Member

lehni commented Dec 13, 2019

This is blocking the release of a new version… We should also implement a unit test for whatever was broken here.

@Filprots
Copy link
Contributor

Hello. I've made a sketch representing the broken example.
I've uncovered this bug, so i'll be happy if it'll help you to solve it.

SKETCH

@lehni lehni closed this as completed in c82e5d4 Dec 14, 2019
@sasensi
Copy link
Contributor Author

sasensi commented Dec 15, 2019

@lehni, sorry for the delay, I am overflowed by work currently.
Was your last commit (that closed this thread) able to fix the issue ?

@lehni
Copy link
Member

lehni commented Dec 15, 2019

@sasensi no worries, I know too well how that goes ;) Yes, my change fixes the new bug as well as the original issue here. Your test is still there and passes. Should be all good!

@lehni
Copy link
Member

lehni commented Dec 15, 2019

@sasensi …and I added a test for the new issue here:

c82e5d4#diff-d0910dfd0a27fc801d12c43b3c3e040fR202

@sasensi
Copy link
Contributor Author

sasensi commented Dec 15, 2019

@lehni, ok, great, thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat: matrix status: PR proposed an active pull request should fix this issue type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants