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

Refactor machine ownership to use player uuid #2905

Open
wants to merge 18 commits into
base: 1.20.1
Choose a base branch
from

Conversation

omergunr100
Copy link
Contributor

@omergunr100 omergunr100 commented Feb 24, 2025

What

Stores placer uuid instead of ownership object.
Ownership objects still exist but are generated in the getter.

Outcome

This should solve some edge cases in our current implementation:

  1. machines would cache teams that no longer exist.
  2. when a player leaves the team the machine ownership object would keep both the team and player even though one should be invalid. (in this implementations machines belong to the player first and team second)

Potential Compatibility Issues

Will remove all machine ownership data from existing worlds.
Moved ownership from block entity to meta machine so...
Any addon that re-assigned ownership will need to change from machine owner object to player uuid.

@omergunr100 omergunr100 added the type: refactor Suggestion to refactor a section of code label Feb 24, 2025
…ing' into oe/ownership-refactor

# Conflicts:
#	src/main/java/com/gregtechceu/gtceu/common/machine/owner/ArgonautsOwner.java
#	src/main/java/com/gregtechceu/gtceu/common/machine/owner/FTBOwner.java
@omergunr100 omergunr100 marked this pull request as ready for review February 24, 2025 08:37
@omergunr100 omergunr100 requested a review from a team as a code owner February 24, 2025 08:37
@omergunr100 omergunr100 added the Merge on Major Release Breaking changes, must be bundled into an 1.X.0 Update label Feb 24, 2025
Copy link
Contributor

@krossgg krossgg left a comment

Choose a reason for hiding this comment

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

One further thought - maybe you could create a static UUID->IMachineOwner cache so that you don't have to create new instances of owners every time you call a get. Doesn't need to be persisted to savedata or anything.

@omergunr100
Copy link
Contributor Author

tbh ownership should be moved from the block entity to the machine since the block entity holder can't be synced and there's no reason for the owner field to randomly just sit there.

Copy link
Contributor

@krossgg krossgg left a comment

Choose a reason for hiding this comment

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

Another note, are the IMachineOwner methods save, load, and create (+ impls) still necessary? Seems that the owner itself is no longer serialized unless I missed something.

extract common player information provider to interface.
remove save and load methods.
changed internal method access to private.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge on Major Release Breaking changes, must be bundled into an 1.X.0 Update type: refactor Suggestion to refactor a section of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants