Skip to content

Conversation

tn-lorenz
Copy link

@tn-lorenz tn-lorenz commented Jul 14, 2025

Important

This PR relies on Entity saving to work (which I was under the impression it already does), so I'll mark this as merge ready until then

Description

This PR aims to add the covetted PersitentDataContainer feature to the Plugin API, but there are still some things I need to consider:

  • Add error handling when plugin dev tries to register an already existing key (now emits warning)
  • Implement for more types + actually hook into the type serde
    • Chunk
    • World
    • Entity
    • TileState*
    • Structure*
    • ItemMeta
    • GeneratedStructure*
    • Raid*
    • OfflinePlayer
    • ItemStack

(*) : These are probably impossible to implement right now, because Pumpkin doesn't handle them yet

Testing

Will be done by:

  • Checking in-game
  • Using unit tests

…te macro persistent_data that annotates the field that holds the PDC
…n of the FromPersistentDataType for any PersistentDataType that might be added in the future
@tn-lorenz tn-lorenz requested a review from MatrixFrog July 16, 2025 18:46
@tn-lorenz tn-lorenz marked this pull request as ready for review July 18, 2025 17:29
if let Some(living) = self.get_living_entity() {
living.write_nbt(nbt).await;
living.write_nbt(&mut compound.clone()).await;

Choose a reason for hiding this comment

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

Probably don't use clone here, make write_nbt take ownership so the caller has to clone, making it clearer when clones are happening

if let Ok(ns_key) = NamespacedKey::new(namespace, key) {
let value = match tag {
NbtTag::Byte(b) => PersistentDataType::Bool(*b != 0),
NbtTag::Short(s) => PersistentDataType::I32(i32::from(*s)),

Choose a reason for hiding this comment

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

Why is a short 32 bits here? you already have a U16 type defined above

#[macro_export]
macro_rules! ns_key {
($value:expr) => {
match ::pumpkin::plugin::persistence::NamespacedKey::new(env!("CARGO_PKG_NAME"), $value) {

Choose a reason for hiding this comment

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

If possible, get the macro to generate a literal instead of calling new()

@unschlagbar
Copy link
Contributor

The idea behind this is genius

@Snowiiii
Copy link
Member

@tn-lorenz Conflicts are getting more and more and i don't understand if this is done or not because it is not marked as draft but there are many things missing looking at the Checkboxes, The last commit was also already a while ago

@tn-lorenz
Copy link
Author

tn-lorenz commented Aug 15, 2025

@Snowiiii Well as I pointed out in the important block at the beginning (and more than once on discord), it is currently not possible to go further and test, as NBT Data is never read in some cases. At the time, I verified this by adding debug statements, but only the one corresponding to saving got printed, never the one for reading.
This was further emphasized by the fact that spawned mobs were not persistent between restarts.
Also, I'm heading for vacation now, so don't expect me finishing this in the next 7-9 days.

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.

5 participants