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

Extend Data Attachment API to ProtoChunk #3548

Merged
merged 10 commits into from
Feb 9, 2024

Conversation

jacobsjo
Copy link
Contributor

@jacobsjo jacobsjo commented Jan 22, 2024

This PR extends the Data Attachment API to allow data attachments on ProtoChunks, rather than just WorldChunks. Data on a ProtoChunk is kept when it is converted to a WorldChunk. This allows storing custom data during worldgen and reading it during gameplay.

Individual changes:

  • Move the interface injection from WorldChunk to Chunk.
  • Transfer the attachment of a ProtoChunk to a WorldChunk in the WorldChunk constructor.
    • I've reused the copyOnRespawn helper method for that and renamed it to transfer to better match its extended use.
  • A separate mixin to WrapperProtoChunk that simply forwards the calls to the wrapped WorldChunk.

What still needs to be decided:

  • Should WrapperProtoChunk.setAttached adhere to the propagateToWrapped flag? -> We ignore the flag and always write to the WorldChunk.
  • What should happen when trying to attach a persistent attachment to a ProtoChunk with chunk status EMPTY? Since those aren't saved, the attachment would get lost too. Options are:
    • Save empty ProtoChunks when an attachment is present. (I would still need to investigate how to do that.)
    • Throw an exception when trying to attach a persistent attachment (but allow non-persistent attachments).
      -> We log a warning. The javadoc documents this behavior.

Tests:

I've extended the testmod to test saving data to a ProtoChunks.

  • A custom configured feature that writes an attachment to the ProtoChunk if it is in the [0, 0] chunk, then testing whether the attachment is present in the WorldChunk after world-load. This tests both writing to ProtoChunks and the WorldChunk conversion.
  • To test persistence in ProtoChunks, I'm adding an attachment to a far chunk. This requires this chunk to not be generated beforehand.

- moved interfaceInjection from WorldChunk to Chunk
- dataAttachment saving on ProtoChunks in ChunkSerializer
- copy attachment from ProtoChunk to WorldChunk on creation.
- make WrapperProtoChunk wrap attachment calls to WorldChunk
@modmuss50 modmuss50 added the enhancement New feature or request label Jan 22, 2024
Copy link
Contributor

@Syst3ms Syst3ms left a comment

Choose a reason for hiding this comment

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

Code looks good so far. I don't know too much about how worldgen and chunk code works, so I'm unsure about the design decisions to make. I'd have someone else weigh in on this.

For propagateToWrapped, if that field is named intuitively, it would make sense to always attach the data to the ProtoChunk, and also attach it to the wrapped chunk depending on the flag.

I don't exactly know what EMPTY ProtoChunks correspond to, but I'd suggest simply emitting a warning when trying to use a persistent attachment. An exception is overkill, and manually saving such chunks feels like an intrusive change for no reason.

@jacobsjo
Copy link
Contributor Author

To elaborate further on the 2 open questions:

The propagateToWrapped flag is basically always false, so the question is rather whether WrapperProtoChunks should allow setting attachments at all.

There are basically 2 use cases for writing attachments to ProtoChunks: The first use case is writing to the currently generating chunk during worldgen. For this case, neither of the 2 open questions matter, as we never deal with WrapperProtoChunks, nor with chunk status EMPTY. Maybe you'd want to write to neighboring chunks, but even then you
probaby won't encounter a WrapperProtoChunks unless someone was manually pruning chunks. In that case it might be confusing if setting blocks is silently ignored but writing attachments is allowed.

The second use case is saving to chunks at arbitrary positions, i.e. writing something like:

overworld.getChunkManager().getChunk(x, z, ChunkStatus.EMPTY, true).setAttached(...);

If we want to support this use case (and I could very well live with a "no" to this questions), then the questions become more relevant:

  • Writing to ProtoChunks with EMPTY chunk would be helpful, but just exchanging EMPTY with STRUCTURE_STARTS (the very first actual generation state) just works without changes. (And is what I currently do in the testmod)
  • Writing in WrapperProtoChunks would be helpful as then you don't need to differentiate between chunks that are fully generated and those that aren't

@jacobsjo
Copy link
Contributor Author

I've extended the testmod to include a direct test of writing to protochunks during worldgen, which is the main use case of this PR.

@Syst3ms
Copy link
Contributor

Syst3ms commented Jan 24, 2024

Writing to ProtoChunks with EMPTY chunk would be helpful, but just exchanging EMPTY with STRUCTURE_STARTS (the very first actual generation state) just works without changes. (And is what I currently do in the testmod)

That sounds like a decent solution, please document it in AttachmentTarget in an extra section. Don't forget to also update the references to WorldChunk that are already present.

@modmuss50 modmuss50 added the last call If you care, make yourself heard right away! label Jan 28, 2024
@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Feb 9, 2024
@modmuss50 modmuss50 merged commit 32782cf into FabricMC:1.20.4 Feb 9, 2024
5 checks passed
modmuss50 pushed a commit that referenced this pull request Feb 9, 2024
* allow data-attachment on ProtoChunks

- moved interfaceInjection from WorldChunk to Chunk
- dataAttachment saving on ProtoChunks in ChunkSerializer
- copy attachment from ProtoChunk to WorldChunk on creation.
- make WrapperProtoChunk wrap attachment calls to WorldChunk

* add test for data-attachment on ProtoChunks, and extend testmod.

* code style and license headers

* fix typos in javadoc

* extend testmod to test setting attachment during worldgen.

* code formatting

* fix testmod: don't crash when feature isn't placed (i.e. on GameTest server)

* add warning when adding persistent attachment to chunk with status EMPTY.

* update javadoc

* update javadoc to reference ServerLivingEntityEvents#MOB_CONVERSION

(cherry picked from commit 32782cf)
@jacobsjo jacobsjo deleted the protoChunk-data-attachment branch February 9, 2024 21:10
Syst3ms pushed a commit to Syst3ms/fabric that referenced this pull request Feb 10, 2024
* allow data-attachment on ProtoChunks

- moved interfaceInjection from WorldChunk to Chunk
- dataAttachment saving on ProtoChunks in ChunkSerializer
- copy attachment from ProtoChunk to WorldChunk on creation.
- make WrapperProtoChunk wrap attachment calls to WorldChunk

* add test for data-attachment on ProtoChunks, and extend testmod.

* code style and license headers

* fix typos in javadoc

* extend testmod to test setting attachment during worldgen.

* code formatting

* fix testmod: don't crash when feature isn't placed (i.e. on GameTest server)

* add warning when adding persistent attachment to chunk with status EMPTY.

* update javadoc

* update javadoc to reference ServerLivingEntityEvents#MOB_CONVERSION
modmuss50 pushed a commit to Syst3ms/fabric that referenced this pull request Feb 13, 2024
* allow data-attachment on ProtoChunks

- moved interfaceInjection from WorldChunk to Chunk
- dataAttachment saving on ProtoChunks in ChunkSerializer
- copy attachment from ProtoChunk to WorldChunk on creation.
- make WrapperProtoChunk wrap attachment calls to WorldChunk

* add test for data-attachment on ProtoChunks, and extend testmod.

* code style and license headers

* fix typos in javadoc

* extend testmod to test setting attachment during worldgen.

* code formatting

* fix testmod: don't crash when feature isn't placed (i.e. on GameTest server)

* add warning when adding persistent attachment to chunk with status EMPTY.

* update javadoc

* update javadoc to reference ServerLivingEntityEvents#MOB_CONVERSION

Backport persistent state code
modmuss50 pushed a commit that referenced this pull request Feb 13, 2024
* allow data-attachment on ProtoChunks

- moved interfaceInjection from WorldChunk to Chunk
- dataAttachment saving on ProtoChunks in ChunkSerializer
- copy attachment from ProtoChunk to WorldChunk on creation.
- make WrapperProtoChunk wrap attachment calls to WorldChunk

* add test for data-attachment on ProtoChunks, and extend testmod.

* code style and license headers

* fix typos in javadoc

* extend testmod to test setting attachment during worldgen.

* code formatting

* fix testmod: don't crash when feature isn't placed (i.e. on GameTest server)

* add warning when adding persistent attachment to chunk with status EMPTY.

* update javadoc

* update javadoc to reference ServerLivingEntityEvents#MOB_CONVERSION

Backport persistent state code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants