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

Make visual effects serializable #7114

Open
wants to merge 4 commits into
base: dev/feature
Choose a base branch
from

Conversation

Romitou
Copy link
Sponsor Member

@Romitou Romitou commented Sep 26, 2024

Description

Started by @TPGamesNL in PR #4123 (the commit has been cherry-picked and the author has been kept for credits), providing a serializer for VisualEffectType allows to serialize VisualEffect correctly. In fact, this should have been handled by simpleClassLoaders, where this specific class was originally registered.

However, the Yggdrasil#getSerializer method did not return a serializer: a SimpleClassResolver extends from ClassResolver and not from a YggdrasilSerializer like the others. A fix is therefore to directly register this serializer at the root of classResolvers.

https://github.com/SkriptLang/Skript/blob/master/src/main/java/ch/njol/yggdrasil/Yggdrasil.java


Target Minecraft Versions: any
Requirements: none
Related Issues: #4196

@Efnilite
Copy link
Member

also you can remove the license from the top of the files

@Romitou Romitou force-pushed the feature/yggdrasil-visual-effect branch from d95d3cb to b2c231e Compare September 26, 2024 21:25
@Efnilite Efnilite added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 26, 2024
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Does this handle types with data, like vibrations or dust particles? Also, I'd like to see some regression tests if possible.

@Override
public Fields serialize(VisualEffectType visualEffectType) {
Fields fields = new Fields();
fields.putObject("id", visualEffectType.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

doing a quick look thru the VisualEffectType code, I see this regarding IDs

public String getId() {
	return effect.getDeclaringClass().getSimpleName() + "." + effect.name();
}

The issue here is the ID is the class name and enum name.
Problem comes from the fact Mojang has changed particle names several times in the last few versions.
And as of I believe 1.21 Bukkit has changed a whole bunch of the enums.
That said, this is not version safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants