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

Update to latest SpongeAPI 11.0.0 #547

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

pschichtel
Copy link
Contributor

@pschichtel pschichtel commented May 30, 2024

for that it also does:

  • update the sponge module to Java 21+
  • update the shadow plugin in sponge to 8.1.1 (for Java 21 support)
  • update the sponge gradle plugin to 2.2.0
  • Replace ServerSideConnectionEvent.Disconnect by ServerSideConnectionEvent.Leave
  • Remove the usage of GameModes.NOT_SET
  • comment-in the light code as it is implemented in sponge these days

The API update was necessary now, because SpongeAPI finally made a breaking API change that affects BlueMap.

I'd argue this cannot be merged as-is because it currently depends on an unreleased version of the SpongeAPI (11.0.0-SNAPSHOT). I brought the lack of releases up with the sponge team, but not sure when a release will be available, probably not very soon. In the meantime I'll maintain my fork/this PR as necessary.

What are your thoughts on how to integrated this? I considered duplicating the sponge module into a sponge-11 module as done by other implementations.

@pschichtel
Copy link
Contributor Author

/cc @Faithcaio

@TBlueF
Copy link
Member

TBlueF commented May 30, 2024

Until now, (as there was not much activity around bluemap's SpongeAPI implementation anyways) I was just keeping the supported version on the SpongeAPI-Version that has an implementation with a recommended release (v8.2 / 1.16.5).

However recently I was planning to consider updating the sponge-implementation to the latest released API-Version instead (which is v10 right now iirc), since sponge doesn't seem to get a new recommended implementation anytime soon and it felt like the snapshot versions are used by quite some people as well. (not sure about the actual metrics)

The next thing planned is to merge #537, which already includes some things like updating java, the shadow-plugin and gradle. So there might be some upcoming merge-conflicts there ^^
Right now i'll be focusing on getting that version stable, and after that i am ready to think about which sponge-version to support :D

@pschichtel
Copy link
Contributor Author

Sure, no rush here. Would be great to get a comment here once you merged the 5.x branch, then I'll rebase this.

@TBlueF
Copy link
Member

TBlueF commented Jun 11, 2024

The 5.x branch is merged now, and i updated the sponge-implementation to SpongeAPI 10.
I would update to SpongeAPI 11 as soon as there is a recommended release :)

@pschichtel pschichtel force-pushed the feature/sponge-api-11 branch from 0b22dec to 196ca0f Compare June 11, 2024 22:49
@pschichtel
Copy link
Contributor Author

@TBlueF I rebased this, the only change left is the Disconnect -> Leave switch. I did bring up the release issue with sponge. I'll maintain this as needed.

Does CI pass? I wasn't able to build with fabric locally due to some java version mismatch.

@pschichtel
Copy link
Contributor Author

@TBlueF how about this one (second commit) ? I switched it back to 8.2.0 and used reflection to register a listener for either the Disconnect or Leave event, depending on which one exists). This way the plugin would be compatible with all the SpongeAPI versions.

@pschichtel pschichtel force-pushed the feature/sponge-api-11 branch 2 times, most recently from f921a38 to 8304404 Compare June 13, 2024 17:00
@TBlueF
Copy link
Member

TBlueF commented Jun 14, 2024

To be honest i want to go away from supporting all the range of individual versions for each platform with each BlueMap release.
This has become annoying and limiting too much ^^
I want to go towards supporting the most-recent recommended version for each platform.

For sponge this means supporting API 10 right now until API 11 is "recommended" and then dropping API 10 support and
switching to API 11.
If people need a version for an older version, then they will have to use an older BlueMap version that still supports it. Just like other plugins do this as well :D

Having to use reflection to support as much as possible API's at once just doesn't feel nice to me and is hard to maintain..
So i would prefer to not do that :)

@pschichtel
Copy link
Contributor Author

@TBlueF yeah I fully get that. it was just an idea I had because I thought that it's kinda unfortunate to lose the backwards compat just for this tiny API change. The API 11 release might not be too far out, so I'll drop the second commit and wait for that.

@pschichtel pschichtel force-pushed the feature/sponge-api-11 branch from 8304404 to c74eead Compare June 16, 2024 15:39
for that it also does:
* Replace `ServerSideConnectionEvent.Disconnect` by `ServerSideConnectionEvent.Leave`
@pschichtel pschichtel force-pushed the feature/sponge-api-11 branch from c74eead to 39132f2 Compare June 16, 2024 20:53
@pschichtel pschichtel changed the title Update to latest SpongeAPI 11.0.0-SNAPSHOT Update to latest SpongeAPI 11.0.0 Jun 16, 2024
@pschichtel
Copy link
Contributor Author

@TBlueF API 11 is now released and I updated the PR accordingly.

@TBlueF TBlueF merged commit 925f9bb into BlueMap-Minecraft:master Jun 17, 2024
2 checks passed
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