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

Fix NotSerializableException #3346

Closed
wants to merge 8 commits into from
Closed

Fix NotSerializableException #3346

wants to merge 8 commits into from

Conversation

APickledWalrus
Copy link
Member

@APickledWalrus APickledWalrus commented Aug 29, 2020

Description

This PR fixes the NotSerializableException when PersistentDataHolder is a registered ClassInfo. Relational Variables are also re-enabled.

I'm not sure whether this is a good solution or if my terminology is correct haha - so let me know ;D

The check will throw an assertion error, but that doesn't seem to be related to this.

EDIT: Marking this as a draft for now because there are still some related issues.


Target Minecraft Versions: 1.14+
Requirements: None
Related Issues: #3185

Also enables Relational Variables again
@APickledWalrus APickledWalrus added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Aug 29, 2020
@APickledWalrus APickledWalrus marked this pull request as draft August 30, 2020 00:28
Seems to work without issue, but prints an error stating that PersistentDataHolder's'serializeAs' class is not serializable
@APickledWalrus APickledWalrus marked this pull request as ready for review September 1, 2020 04:17
@APickledWalrus
Copy link
Member Author

This fix should work better than the last (no variable loading issue).

However, I don't know if this solution is safe *will it cause other issues?).
While I believe everything works fine, the following error also occurs:

[ERROR] [Skript] persistentdataholder's 'serializeAs' class is not serializable 

@@ -113,7 +113,7 @@ public static void onRegistrationsStop() {
final ClassInfo<?> sa = getExactClassInfo(ci.getSerializeAs());
if (sa == null) {
Skript.error(ci.getCodeName() + "'s 'serializeAs' class is not registered");
} else if (sa.getSerializer() == null) {
} else if (sa.getSerializer() == null && ci.getSerializeAs() != Object.class) {
Copy link
Member

Choose a reason for hiding this comment

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

do you think it would be a good idea to only evaluate this second part if persistent data exists? it may help limit impact if this ends up causing a different issue

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t think it will matter really. No other ClassInfos should really be using Object here unless they encounter this specific issue. Maybe I’m misunderstanding what you mean though?

@APickledWalrus APickledWalrus added this to the 2.6 milestone Sep 24, 2020
@APickledWalrus
Copy link
Member Author

Setting this to target 2.6 for now, as I want to do more extensive testing.

@ShaneBeee ShaneBeee added the 2.6 label Sep 24, 2020
@APickledWalrus
Copy link
Member Author

reopening & updating this because I made a new fork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants