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

Paths not visible when children of Groups #315

Closed
johnoneil opened this issue Aug 24, 2023 · 5 comments
Closed

Paths not visible when children of Groups #315

johnoneil opened this issue Aug 24, 2023 · 5 comments
Assignees
Labels
🟢 P2 Priority for next 2-3 releases Type: Bug Something isn't working

Comments

@johnoneil
Copy link
Contributor

I've been struggling on this issue for a couple of days, so I'll try to describe it methodically. I think I've got it pinned down, but I could be wrong.

I'm currently working on a fix and may present that shortly. I'm open to any questions or suggestions to debug/further explore this issue.

it's also possible there may be a fix on the client side. Not 100% sure yet. I'll take that option if possible.

I believe this issue may be at least partially related to issue #244 . A fix for this issue may also help clarify that (and hopefully not break things!).

First, this is the Figma element we're dealing with. There is a parent node, then a child group (g24) which is offset from the parent, and then a bunch of child paths.

Screenshot from 2023-08-24 11-29-07

When I render this on the client side, using the path absolute position for translation, and the (accumulated) parent transforms, we see an offset creeping into the paths. Like in the following image.

Screenshot from 2023-08-24 11-26-57

As you can see from the image, the individual paths are drawn correctly, and their general arrangement is okay, but they are offset incorrectly.

I believe the origin of this issue is this section in transform_flexbox.rs

            let r = r.post_translate(euclid::vec3(
                -(bounds.x() - parent_bounds.x()),
                -(bounds.y() - parent_bounds.y()),
                0.0,
            ));

This section tries to remove translation from the relative_transform matrix provided by figma. That is, it removes (leading minus signs) the offset from the local rect bounds from its parent.

However there are cases when this will be incorrect. I'm referring to this doc here:

https://www.figma.com/plugin-docs/api/properties/nodes-relativetransform/#:~:text=The%20relative%20transform%20of%20a,group%20or%20a%20boolean%20operation.

Note the section:

The relative transform of a node is shown relative to its container parent, which includes canvas nodes, frame nodes, component nodes, and instance nodes. Just like in the properties panel, it is not relative to its direct parent if the parent is a group or a boolean operation.

So in our case, since the immediate parent is a Group (g24) the translation data present in the relative_transform matrix will not be entirely removed. Instead we should be "removing" translation data from the (further offset!) goup g24's parent.

Okay I'm going to try out fixes for this, but I think the approach will be to carry around not just the parent but also a last_relative_transform_parent as we traverse the tree. In most cases these will be the same node, but in these edge cases they will be different.

Will update when I have more.

@github-project-automation github-project-automation bot moved this to Needs Triage in DesignCompose Aug 24, 2023
@johnoneil johnoneil changed the title Style transform member translation information can be incorrect when parent is a group/boolean. ViewStyle transform member translation information can be incorrect when parent is a group/boolean. Aug 24, 2023
@johnoneil johnoneil changed the title ViewStyle transform member translation information can be incorrect when parent is a group/boolean. Paths not visible when children of Groups Aug 25, 2023
@johnoneil
Copy link
Contributor Author

Update:

Okay in a Figma test doc I added some cases reproducing this issue. Note the red "airbag" telltales near the bottom:

Screenshot from 2023-08-25 16-45-41

But when I render in the validation-app I see the following (this is on main).

Screenshot from 2023-08-25 16-47-20

So I'm calling this bug Paths not visible when children of Groups.

With a fix to the issue as described above (adjusting the transform member to remove translations from the correct parent`) the issue was not fixed.

I believe this will take a deeper dive into the .kt renderer to see why this is the case.

Generally I'm going to recommend not adjusting transforms coming out of Figma. I'm taking that approach currently and am finding it somewhat simpler. In other words, maybe we could transition the transform member to take the unadjusted relative_transform data.

github-merge-queue bot pushed a commit that referenced this issue Aug 28, 2023
This is to address the issue I saw and filed as #315.

The primary change is to include the original unaltered
`relative_transform` in the serialized figma output. Since this addition
is merely additive, that change should not break anything.

~~**However** I'm also making a correction to the `transform`
calculation which I believe to be correct, but which could break
downstream clients. Maybe we can talk about how to test this.~~

The addition of the original `relative_transform` in the serialzied
document has allowed me to decrease the complexity of of our renderer
(slighly) and also fixes the original issue. I'll attach a screenshot of
our (non-flattened) telltales below.

![Screenshot from 2023-08-24
23-37-05](https://github.com/google/automotive-design-compose/assets/2950232/54c2fbe4-f8f8-4d96-9c93-ac2d43ced24f)

---------

Co-authored-by: John O'Neil <[email protected]>
@johnoneil
Copy link
Contributor Author

Just to update this bug.

The change I landed above allows the current Rust (impeller based) renderer to work around this bug, but it's still present in Android.

I thought I had a correction that should fix the Android side as well, but that simply didn't work (see the screenshots above). I believe it will take some deeper digging to find out why that fix didn't work.

It's probably moot since we ought to be moving towards shared rendering code, so that any (major) differences in renderers should go away. If this issue becomes a priority on Android I can take a look and perhaps help land a fix.

For the moment I'd suggest leaving this bug, and closing this when we land shared rendering code.

timothyfroehlich added a commit that referenced this issue Sep 5, 2023
@timothyfroehlich
Copy link
Member

Let's get this reproduced and in Validation so that we can confirm that it's fixed at some point

@timothyfroehlich
Copy link
Member

@johnoneil Do you still have this reproduced somewhere?

@timothyfroehlich timothyfroehlich added Type: Bug Something isn't working 🟢 P2 Priority for next 2-3 releases labels Sep 28, 2023
@johnoneil
Copy link
Contributor Author

Yikes. I think this has been resolved for some time. Sorry to leave it open.
Closing as we haven't seen this in a while.

@github-project-automation github-project-automation bot moved this from Needs Triage to Done in DesignCompose Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟢 P2 Priority for next 2-3 releases Type: Bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

2 participants