Skip to content

1.21.4#4582

Merged
NotMyFault merged 14 commits intomainfrom
fix/1.21.4
Feb 23, 2025
Merged

1.21.4#4582
NotMyFault merged 14 commits intomainfrom
fix/1.21.4

Conversation

@PierreSchwang
Copy link
Member

Overview

Not tested yet, checked IDs against minecraft wiki and seems to work out.

### Submitter Checklist
- [x] Make sure you are opening from a topic branch (**/feature/fix/docs/ branch** (right side)) and not your main branch.
- [x] Ensure that the pull request title represents the desired changelog entry.
- [x] New public fields and methods are annotated with `@since TODO`.
- [x] I read and followed the [contribution guidelines](https://github.com/IntellectualSites/.github/blob/main/CONTRIBUTING.md).

@PierreSchwang PierreSchwang requested a review from a team as a code owner January 25, 2025 20:19
@github-actions github-actions bot added the Bugfix This PR fixes a bug label Jan 25, 2025
@PierreSchwang PierreSchwang mentioned this pull request Jan 25, 2025
@NotMyFault NotMyFault requested a review from a team February 12, 2025 20:02
@dordsor21
Copy link
Member

dordsor21 commented Feb 15, 2025

Do all these new classes/fields exist in 1.19.4? We still technically target it for support in P2 I believe?

@PierreSchwang
Copy link
Member Author

I think I should look at the PR again, as there are seemingly string constants for entity types in the code, which obviously weren't caught by the newer paper api

@PierreSchwang
Copy link
Member Author

I'm not gonna touch the ReplicatingEntityWrapper (required for plot clear as it seems?). That would need a whole refactor for maaany things (and potentially a migration to just storing the entities as NBT - but that's something that has to be discussed).

@dordsor21
Copy link
Member

I'm not gonna touch the ReplicatingEntityWrapper (required for plot clear as it seems?). That would need a whole refactor for maaany things (and potentially a migration to just storing the entities as NBT - but that's something that has to be discussed).

Does it need a refactor though? What needs refactoring?

@PierreSchwang
Copy link
Member Author

I'm not gonna touch the ReplicatingEntityWrapper (required for plot clear as it seems?). That would need a whole refactor for maaany things (and potentially a migration to just storing the entities as NBT - but that's something that has to be discussed).

Does it need a refactor though? What needs refactoring?

Well, without looking further, the first things I noticed:

  • many entity types are not supported / added
  • only BOAT is currently added (not the individual ones, like OAK_BOAT etc), setting the dataByte to the ordinal value of getWoodType of TreeVariant (which does not support all supported wood types)
  • Chest-Boats are not supported at all (no inventory, no dataByte)
  • Rafts are missing

I guess I could add the named ones, but it would be really great to get to java 21 to use type pattern matching in the switch-cases (though, blocked by SpigotMC atm)

@dordsor21
Copy link
Member

I'm not gonna touch the ReplicatingEntityWrapper (required for plot clear as it seems?). That would need a whole refactor for maaany things (and potentially a migration to just storing the entities as NBT - but that's something that has to be discussed).

Does it need a refactor though? What needs refactoring?

Well, without looking further, the first things I noticed:

  • many entity types are not supported / added
  • only BOAT is currently added (not the individual ones, like OAK_BOAT etc), setting the dataByte to the ordinal value of getWoodType of TreeVariant (which does not support all supported wood types)
  • Chest-Boats are not supported at all (no inventory, no dataByte)
  • Rafts are missing

I guess I could add the named ones, but it would be really great to get to java 21 to use type pattern matching in the switch-cases (though, blocked by SpigotMC atm)

Perhaps just minimal work for 1.21.4 and we can revisit this later? I don't see much issue with using strings - multi-version support is tricky otherwise.

@dordsor21
Copy link
Member

Looks good enough for now to me

@NotMyFault NotMyFault merged commit d4f1042 into main Feb 23, 2025
10 checks passed
@NotMyFault NotMyFault deleted the fix/1.21.4 branch February 23, 2025 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugfix This PR fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments