Skip to content

Fixed visibility property for visuals with shadow - #492

Merged
aborziak-ms merged 5 commits into
mainfrom
fix-shadow-effect
Feb 28, 2022
Merged

aborziak-ms merged 5 commits into
mainfrom
fix-shadow-effect

Conversation

@aborziak-ms

Copy link
Copy Markdown
Collaborator

Fixing translation of shadow effect. If source of the shadow is not visible it does not make shadow disappear, so here I'm pushing IsVisible property up so that this property controls visibility of both, source and shadow visuals.

@aborziak-ms
aborziak-ms requested a review from a team February 23, 2022 23:50
Comment thread source/LottieToWinComp/Effects.cs Resolved
Comment thread source/LottieToWinComp/Effects.cs Outdated
}

// Here we are moving default value of "IsVisible" to target
target.IsVisible = source.IsVisible;

@raymond-wh-leung raymond-wh-leung Feb 24, 2022 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

target.IsVisible = source.IsVisible;

If the Animate.WithKeyFrame() or WithExpression() calls StartAnimation() that targets IsVisibile, this set here (if the the value gets changed), will stop the animation above, maybe this needs to be set first before the Animate.WithKeyFrame() calls above? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This class is just a "mock class" of a real WinComp class (Lottie-Windows\source\WinCompData\Visual.cs) so it just stores the data in it, after all the translation is done this class is used to instantiate real WinComp instances. So it should be fine, but I reordered it anyway

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Gotcha.

Comment thread source/LottieToWinComp/Effects.cs Outdated
// We need to transfer "IsVisible" property from source to result
// because result is now the root visual for both shadow and source.
// Otherwise "IsVisible" property will be applied only to source.
MoveIsVisibleProperty(context, source, result);

@raymond-wh-leung raymond-wh-leung Feb 24, 2022 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MoveIsVisibleProperty

nit: what do you think about TransferIsVisible or PropagateIsVisible? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Transfer sounds good, done

@raymond-wh-leung raymond-wh-leung left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:shipit:

@aborziak-ms
aborziak-ms merged commit 109463c into main Feb 28, 2022
@delete-merged-branch
delete-merged-branch Bot deleted the fix-shadow-effect branch February 28, 2022 19:25
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.

2 participants