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

Basic animation support for decorators. #421

Merged
merged 11 commits into from
Mar 7, 2023
Merged

Basic animation support for decorators. #421

merged 11 commits into from
Mar 7, 2023

Conversation

0suddenly0
Copy link
Contributor

Since decorators can use properties of standard types, they can also be animated, as well as basic properties.

@mikke89 mikke89 added the enhancement New feature or request label Feb 28, 2023
@mikke89
Copy link
Owner

mikke89 commented Feb 28, 2023

Hey, this is very cool. Although, I have actually implemented this very feature already in another branch: 8023d31.

I'd be willing to merge this in the meantime though, while we wait for the above branch to be merged. Perhaps you could take a look at that and see if there are any ideas or improvements you could make from it.

I've also written some unit tests here: 940b62f. It would be nice if you could add something like this. This one uses filter property instead of decorators which is not available in the master branch, but the idea should be the same.

@0suddenly0
Copy link
Contributor Author

Thanks for the reply. I think your solution from the filter branch will be more correct than mine, the only drawback is the lack of support for decorator at-rule from your solution. In the near future I will try to combine these two solutions and supplement them with tests.

@0suddenly0
Copy link
Contributor Author

I'm not sure, but maybe I should create a separate pull request to add decorator at-rule support to the filter branch, instead of copying your code to my branch?

@mikke89
Copy link
Owner

mikke89 commented Mar 2, 2023

I would rather that you copied the code over to this PR. The filter branch is quite experimental, and will probably have to be re-implemented from scratch. So I'll deal with merging them later when it comes to that.

@0suddenly0
Copy link
Contributor Author

Basically, that's all I could do. Perhaps the tests are not so correct and complete, because I am dealing with their writing for the first time, but in general everything should work correctly.

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Nice job. I just have a few minor comments.

The tests look good to me. I wonder if it would make more sense to make the default gradient color transparent, but in any case, that is a completely different issue.

Could you perhaps add me as a co-author, if you don't mind?

Source/Core/ElementAnimation.cpp Show resolved Hide resolved
Source/Core/ElementAnimation.cpp Outdated Show resolved Hide resolved
Source/Core/ElementAnimation.cpp Outdated Show resolved Hide resolved
Co-Authored-By: Michael R. P. Ragazzon <mikke89@users.noreply.github.com>
@0suddenly0
Copy link
Contributor Author

Nice job. I just have a few minor comments.

The tests look good to me. I wonder if it would make more sense to make the default gradient color transparent, but in any case, that is a completely different issue.

Could you perhaps add me as a co-author, if you don't mind?

I was actually thinking about how to get around creating new copies of Decorator Declaration. To bypass, we can use pointers to specific variables and change them depending on the situation, because their lifetime does not depend on animations. Of course, you can get Decorator Specification separately and check their compatibility through checks, but this option seems simpler to me.

I tried to add you as a co-author, let me know if I did it wrong.

@mikke89
Copy link
Owner

mikke89 commented Mar 6, 2023

I was actually thinking about how to get around creating new copies of Decorator Declaration. To bypass, we can use pointers to specific variables and change them depending on the situation, because their lifetime does not depend on animations. Of course, you can get Decorator Specification separately and check their compatibility through checks, but this option seems simpler to me.

Yeah, the approach seems good, the underlying lifetimes should be fine. Although I'm finding it quite a bit harder to follow this new code. Maybe it would help to abstract away the pointers into something like a simple DecoratorDeclarationView struct?

I tried to add you as a co-author, let me know if I did it wrong.

Looks good, thanks.

@0suddenly0
Copy link
Contributor Author

I was actually thinking about how to get around creating new copies of Decorator Declaration. To bypass, we can use pointers to specific variables and change them depending on the situation, because their lifetime does not depend on animations. Of course, you can get Decorator Specification separately and check their compatibility through checks, but this option seems simpler to me.

Yeah, the approach seems good, the underlying lifetimes should be fine. Although I'm finding it quite a bit harder to follow this new code. Maybe it would help to abstract away the pointers into something like a simple DecoratorDeclarationView struct?

I tried to add you as a co-author, let me know if I did it wrong.

Looks good, thanks.

struct DecoratorDeclarationView {
	DecoratorDeclarationView(const DecoratorDeclaration& declaration) : type(&declaration.type), instancer(declaration.instancer), properties(&declaration.properties) {}
	DecoratorDeclarationView(const DecoratorSpecification* specification) : type(&specification->decorator_type), instancer(Factory::GetDecoratorInstancer(specification->decorator_type)), properties(&specification->properties) {}
	const String* type;
	DecoratorInstancer* instancer;
	const PropertyDictionary* properties;
};

Do you mean something like this?

@mikke89
Copy link
Owner

mikke89 commented Mar 6, 2023

Yeah, at least with those members. The rest I leave to you, if you find it doesn't improve things, then that's okay too.

@mikke89
Copy link
Owner

mikke89 commented Mar 7, 2023

This looks a lot better to me.

Do you feel finished with this? If so I will squash and merge this.

@0suddenly0
Copy link
Contributor Author

0suddenly0 commented Mar 7, 2023

Do you feel finished with this? If so I will squash and merge this.

Yes, I think I have nothing more to add or fix.

@mikke89 mikke89 merged commit e850073 into mikke89:master Mar 7, 2023
@mikke89
Copy link
Owner

mikke89 commented Mar 7, 2023

Sounds good, thanks for the pull request!

igorsegallafa pushed a commit to igorsegallafa/RmlUi that referenced this pull request Mar 12, 2023
Co-authored-by: Michael R. P. Ragazzon <mikke89@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants