-
Notifications
You must be signed in to change notification settings - Fork 82
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
New Sharables #363
base: main
Are you sure you want to change the base?
New Sharables #363
Conversation
I've made it so that the ANNOUNCE_ADVANCEMENTS gamerule on the world the player is travelling to gets set to false if wasn't already, then after updating the player's advancements it's set back to what it was previously, if it was changed at all. This isn't a perfect solution as the player still sees the notifications in the top right of their screen, but it isn't announced in chat. I've requested a way to do that hide those notifications here. |
There is a new Paper PR which may affect the Advancement sharable, see PaperMC/Paper#3454. If this PR gets merged and ends up affecting the behaviour of the sharable, we'd probably have to use PaperLib to fix it. The case I have in mind is if the player is teleported as they join, the sharable may need to be saved, but this may happen before the advancements are loaded. This may never happen, I'm just speculating. |
@nicegamer7 This PR is no longer affected by PaperMC/Paper#3454 as it will now block in the API if it is still loading the data. |
I just cleaned up this PR, redoing some bits and making the commits more specific. The biggest difference is that the advancement sharable no longer needs a custom serializer. I guess this is ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's cool idea!
2f81b5b
to
302e120
Compare
I just rebased this PR onto
|
src/main/java/com/onarandombox/multiverseinventories/share/Sharables.java
Outdated
Show resolved
Hide resolved
src/main/java/com/onarandombox/multiverseinventories/share/Sharables.java
Show resolved
Hide resolved
src/main/java/com/onarandombox/multiverseinventories/share/Sharables.java
Show resolved
Hide resolved
src/main/java/com/onarandombox/multiverseinventories/share/Sharables.java
Outdated
Show resolved
Hide resolved
src/main/java/com/onarandombox/multiverseinventories/share/Sharables.java
Show resolved
Hide resolved
src/main/java/com/onarandombox/multiverseinventories/share/Sharables.java
Outdated
Show resolved
Hide resolved
src/main/java/com/onarandombox/multiverseinventories/share/Sharables.java
Outdated
Show resolved
Hide resolved
Also, note that when switching groups, the advancements are not only announced, but the player is given a certain amount of XP, which causes access to free XP generation by switching groups. An unrelated question - are there plans to also introduce sharing datapacks for the worlds? It'd be awesome if groups could be assigned certain advancement trees ;) (e.g. having a skyblock map with it's own advancements) |
I... did not think of that... Thanks for bringing that to my attention. With regards to datapacks, there's currently no way for MV to interact with them. So at this time, it's not possible to change any datapack behaviour. |
Here's a new build that should be a bit more efficient, and it includes a simple attempt at fixing the XP exploit described above.
|
And once more, a new build with another attempt at fixing the XP exploit. edit: the exploit is fixed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some performance suggestions
Map<String, Integer> playerStats = profile.get(GAME_STATISTICS); | ||
for (Statistic stat : Statistic.values()) { | ||
if (stat.getType() == Statistic.Type.UNTYPED) { | ||
player.setStatistic(stat, 0); | ||
} | ||
} | ||
|
||
if (playerStats == null) { | ||
return false; | ||
} | ||
|
||
for (Map.Entry<String, Integer> statInfo : playerStats.entrySet()) { | ||
Statistic stat = Statistic.valueOf(statInfo.getKey()); | ||
if (stat.getType() == Statistic.Type.UNTYPED) { | ||
player.setStatistic(stat, statInfo.getValue()); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing the for loop twice, I'd do something like this:
Map<String, Integer> playerStats = profile.get(GAME_STATISTICS);
if (playerStats == null) {
// Set all to 0
for (Statistic stat : Statistic.values()) {
if (stat.getType() == Statistic.Type.UNTYPED) {
player.setStatistic(stat, 0);
}
}
return false;
}
for (Statistic stat : Statistic.values()) {
if (stat.getType() == Statistic.Type.UNTYPED) {
player.setStatistic(stat, playerStats.getOrDefault(stat.name(), 0));
}
}
return true;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was planning on doing something like this, just haven't gotten around to it yet.
Hey @nicegamer7, I've been beta testing this on a server I'm on based on your latest build and I'm having an issue where occasionally an achievement gets lost as players travel back and forth (for example I lost monster hunter). |
Thanks for testing @reediculous456, one thing is that I don't think the progress of advancement is reset, so it perhaps is a case of achieving that advancement just after teleport? Steps to reproduce will be good, that way we have something to work off. |
Thanks for testing @reediculous456! I'm glad you found this issue, and as ben said, if you have steps to reproduce that would be very helpful. |
So how is this project doing? I would really like a fix for the advancements coming up on world enter, it also spams my discord Minecraft chat with a player getting advancements... |
I haven't checked, but I'm pretty sure the notifications are client side. If that's the case, there's almost nothing that can be done. If I'm wrong, we'll need to propose an addition to the Bukkit API. |
Hey, i test this version and statistics blocks and mobs continue to shared into both worlds |
This PR adds Advancements, Game Stats, and Recipes Sharables, and thus closes #340.
It's not fully complete yet, as there remains one minor issue: when switching groups, Advancements are announced as though the player is receiving them for the first time.
Other than that, there are some smaller notes I wanted to discuss, such as whether there is a better way of serializing the Advancements, and your thoughts on the sharable's overall efficiency.