8000 Colony match updated and gradle properties by Aaron9174 · Pull Request #10928 · ldtteam/minecolonies · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Colony match updated and gradle properties #10928

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

Conversation

Aaron9174
Copy link
Contributor
@Aaron9174 Aaron9174 commented Jun 8, 2025

Colony match needs to take dimension ID for context. Colony IDs in different dimensions start at 1, so if you have two colonies in different dimensions trying to be allies, they won't show up in the list since the colony ID is not unique. Colony ID + dimension ID is unique and that's what this change involves.

Also, gradle properties setup to run the required version of gradle. Perhaps I have misconstrued the README directions, but I couldn't get this to run with JVM21, and instead got this working with JVM17 + a different gradle version. Possible that the gradle version change can be removed and only the ColonyView change is needed. Has been tested locally and on a server on version 1.20.1 at the most recent tag.

Closes #
Closes #

Changes proposed in this pull request

  • Updating colony ID check for populating ally list to include dimension ID as well
  • Gradle version for build distribution

Testing

  • [ x ] Yes I tested this before submitting it.
  • [ x ] I also did a multiplayer test.

Review please

Colony match needs to take dimension ID for context. Also, gradle
properties setup to run the required version of gradle.
@CLAassistant
Copy link
CLAassistant commented Jun 8, 2025

CLA assistant check
All committers have signed the CLA.

@@ -1,6 +1,6 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-8.1.1-bin.zip
Copy link
Member

Choose a reason for hiding this comment

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

Whenever you change version of gradle wrapper please run twice gradlew wrapper --gradle-version [version]

Copy link
Contributor Author
@Aaron9174 Aaron9174 Jun 10, 2025

Choose a reason for hiding this comment

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

Ah that makes sense, I uh, definitely just wrote this in by hand which isn't the correct way to do it lol
edit: All done

@uecasm
Copy link
Contributor
uecasm commented Jun 9, 2025

JVM21 is only for the 1.21 branch. The main branch is still 1.20 with JVM17.

@Aaron9174
Copy link
8000 Contributor Author

JVM21 is only for the 1.21 branch. The main branch is still 1.20 with JVM17.

This should be updated in the 1.20.1 tagged README then. I will update the README for this branch, so in the next tagged 1.20.1 patch update it will be more accurate.

Updating gradle version properly (use gradlew wrapper instead of hard
coding the version directly in the properties file)
…:Aaron9174/minecolonies into bug/allies-list-empty-on-diff-dimensions
@Aaron9174
Copy link
Contributor Author

All comments and merge conflicts addressed, looking for a re-review here

@@ -377,7 +377,8 @@ public static void serializeNetworkData(@NotNull Colony colony, @NotNull Friendl
{
for (final ColonyPlayer owner : colony.getPermissions().getPlayersByRank(colony.getPermissions().getRankOwner()))
{
if (col.getPermissions().getRank(owner.getID()).isColonyManager() && col.getID() != colony.getID())
boolean colonyMatch = (col.getID() == colony.getID()) && (col.getDimension() == colony.getDimension());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably inline this as && (id != id || dim != dim), no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got it chief, don't mind making the adjustment

Copy link
Contributor Author
@Aaron9174 Aaron9174 Jun 14, 2025

Choose a reason for hiding this comment

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

The change is in, but after re-visiting this, I think it was better before I made the change. As it was more readable via a variable name indicating there was a colonyMatch. Further, the boolean algebra was just simpler to understand before, even with context of these changes, I still had to whip out a truth table to make sure this logic was identical (which it is).

I don't readily see from this statement
col.getID() != colony.getID() || col.getDimID() != colony.getDimID()
that the current colony of the loop doesn't match the ID of the colony sent in the network request.

Truth table confirmed the logic is the same
Raycoms
Raycoms previously approved these changes Jun 14, 2025
@Aaron9174
Copy link
Contributor Author

@Nightenom Got Raycoms approval, mind taking a look at this when you get a chance?

Nightenom
Nightenom previously approved these changes Jun 14, 2025
@Aaron9174
Copy link
Contributor Author

Was building locally wrong, just using the ./gradlew (without build), was just missing an parenthesis and was confused why it worked locally. Committing now.

Missing a closing parenthesis
@Aaron9174 Aaron9174 dismissed stale reviews from Nightenom and Raycoms via 92e2088 June 14, 2025 20:29
@Raycoms Raycoms merged commit cac61d0 into ldtteam:version/main Jun 16, 2025
1 check passed
@Aaron9174 Aaron9174 deleted the bug/allies-list-empty-on-diff-dimensions branch June 17, 2025 01:14
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.

6 participants
0