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

Update docs for capability rework #34

Merged
merged 6 commits into from
Dec 21, 2023
Merged

Conversation

Technici4n
Copy link
Member

@Technici4n Technici4n commented Dec 7, 2023

Mostly adapted from the blog post, but keeping some of the old content where that made sense.

Beyond the two main changed files, I also fixed references to capabilities elsewhere. Some of the other pages will still need an update, but at least everything related to caps should be clean now.


Preview URL: https://pr-34.neoforged-docs-previews.pages.dev

@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 7, 2023 23:43 Active
@neoforged-pages-deployments
Copy link

neoforged-pages-deployments bot commented Dec 7, 2023

Deploying with Cloudflare Pages

Name Result
Last commit: 714a87f7ff02164c11d0b62d1c574f9d7b8b52f5
Status: ✅ Deploy successful!
Preview URL: https://c7514fe0.neoforged-docs-previews.pages.dev
PR Preview URL: https://pr-34.neoforged-docs-previews.pages.dev

@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 7, 2023 23:48 Active
Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 left a comment

Choose a reason for hiding this comment

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

Bunch of general questions because my lack of understanding of the underlying system. I have avoided commenting on syntax changes as much as possible as per your request. They will be addressed at a later point in time since they don't affect the user experience.


To use the system, you need to register an `AttachmentType`.
The attachment type contains:
- a default value supplier to create the instance when the data is first accessed, or to compare stacks that have the data and stacks that don't have it;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only true for stacks, or does it apply to any attached object?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default instance is for any attachment holder object (BE, chunk, entity, stack). Only stacks are comparable so that remark only applies to them, but admittedly we could probably omit it.

docs/datastorage/attachments.md Outdated Show resolved Hide resolved
docs/datastorage/attachments.md Outdated Show resolved Hide resolved
docs/datastorage/attachments.md Outdated Show resolved Hide resolved
docs/datastorage/attachments.md Outdated Show resolved Hide resolved
docs/datastorage/capabilities.md Show resolved Hide resolved
docs/datastorage/capabilities.md Outdated Show resolved Hide resolved
docs/datastorage/capabilities.md Outdated Show resolved Hide resolved
docs/datastorage/capabilities.md Outdated Show resolved Hide resolved
Comment on lines +303 to +305
Providers are asked for a capability in the order that they are registered.
Should you want to run before a provider that NeoForge already registers for one of your objects,
register your `RegisterCapabilitiesEvent` handler with a higher priority.
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

For example if you have a LivingEntity and you want to register a more specific IItemHandler than the one NeoForge already registers.

@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 11, 2023 09:36 Active
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 11, 2023 09:42 Active
Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 left a comment

Choose a reason for hiding this comment

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

Good enough, any formatting errors can be addressed in a future commit. @XFactHD and @Matyrobbrt, will give you until whenever Thursday to do any additional reviews or comments.

@XFactHD
Copy link
Member

XFactHD commented Dec 15, 2023

Looks good to me overall, just a few minor points

  • It might be worth mentioning that the fluid capability for items is slightly special due to the way buckets hold fluids
  • The remark regarding why block caps are looked up from the level now should explicitly mention why (i.e. that blocks without BEs can have caps now as well)
  • If the holder of a data attachment requires it, is it automatically marked dirty when a non-existent attachment is retrieved and therefore default-initialized?

Other than that: :shipit:

@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 21, 2023 14:25 Active
@Technici4n Technici4n merged commit eb25c17 into neoforged:main Dec 21, 2023
1 check passed
@Technici4n Technici4n deleted the cap-rework branch December 21, 2023 14:45
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.

None yet

3 participants