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

Entity docs #171

Open
wants to merge 73 commits into
base: main
Choose a base branch
from

Conversation

IchHabeHunger54
Copy link
Member

@IchHabeHunger54 IchHabeHunger54 commented Oct 3, 2024

Adds some long-needed entity documentation. Supersedes #15 and #95 . Closes #90 . This PR:

  • Adds articles about the following:
    • Entities in general
    • Data and networking in the context of entities
    • The LivingEntity, Mob and Player classes, along with their subclasses
    • Attributes and AttributeModifiers
    • Entity renderers and entity models
  • Removes the Entities article in the Networking section, in favor of the new Data and Networking article
  • Updates the Interaction Pipeline article with left-click and middle-click behavior, and also renames it to just Interactions
  • Adds an explanation for how block breaking speed is calculated, including how the various attributes affect this
    • Also does some reformatting in the blocks article, while we're at it
  • Adds and adjusts links to the new Entities section as necessary
  • Adds a styleguide for Mermaid class diagrams to the Contributing doc
  • Explicitly leaves WIP markers for pathfinding, goals and brains, as well as animations, as these deserve their own PR(s)

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

IchHabeHunger54 and others added 30 commits January 29, 2024 18:04
Co-authored-by: ChampionAsh5357 <[email protected]>
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) November 21, 2024 12:48 Active
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 1, 2024 21:43 Active
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 2, 2024 00:04 Active
@IchHabeHunger54 IchHabeHunger54 marked this pull request as ready for review December 2, 2024 00:10
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 2, 2024 16:15 Active
Copy link
Member

@XFactHD XFactHD left a comment

Choose a reason for hiding this comment

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

First pass over the changes, still have to do the 4 largest files.

docs/blocks/index.md Outdated Show resolved Hide resolved
docs/entities/data.md Outdated Show resolved Hide resolved
docs/entities/data.md Outdated Show resolved Hide resolved
docs/entities/data.md Outdated Show resolved Hide resolved
docs/entities/data.md Outdated Show resolved Hide resolved
Comment on lines +75 to +76
EntityRenderer-->ArrowRenderer;
EntityRenderer-->LivingEntityRenderer;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EntityRenderer-->ArrowRenderer;
EntityRenderer-->LivingEntityRenderer;
EntityRenderer-->LivingEntityRenderer;
EntityRenderer-->ArrowRenderer;

These should be switched around to avoid the arrow from LivingEntityRenderer to ArmorStandRenderer going through the ArrowRenderer node

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this makes the arrow go through the AbstractMinecartRenderer instead. I'm afraid there is no easy way of solving this.

}
```

That's literally it. Extend the class, add your field, and off you go. The only thing left to do now is to update that `stackInHand` field in `EntityRenderer#extractRenderState`, as explained above.
Copy link
Member

Choose a reason for hiding this comment

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

This section should include an overview over the render state modification system introduced in neoforged/NeoForge#1650.

Comment on lines +193 to +196
if (renderer != null) {
// Add the layer to the renderer. Reuses the ModelLayerLocation from above.
renderer.addLayer(MY_LAYER);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong in two ways:

  • This event has nothing to do with LayerDefinitions, it's related to RenderLayers. A RenderLayer however can indeed render an EntityModel created from a LayerDefinition after the latter is baked
  • Only renderers which implement RenderLayerParent can have RenderLayers added to them. In vanilla this only applies to LivingEntityRenderer and its subclasses

RenderLayers should be dealt with in a separate section. This section should instead explain how to go from a LayerDefinition to the custom EntityModel in the renderer's constructor.

// Get the associated PlayerRenderer.
if (event.getSkin(skin) instanceof PlayerRenderer playerRenderer) {
// Add the layer to the renderer.
playerRenderer.addLayer(MY_LAYER);
Copy link
Member

Choose a reason for hiding this comment

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

Same issue as above

docs/entities/data.md Show resolved Hide resolved
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) December 14, 2024 17:12 Active
Comment on lines 71 to 73
// Build the entity type. The parameter is a string used for datafixing; mods generally
// do not utilize this and can safely pass null here instead.
.build(null)
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 accurate? I copied around this code and it complains about

java.lang.NullPointerException: Cannot invoke "String.indexOf(int)" because "location" is null

Going through neo discord, Commoble says that this string cannot be null. https://discord.com/channels/313125603924639766/983834532904042537/1109539085560856656

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be set as it now attempts to resolve the location of the resource key. A patch needs to be added back in to safely supply null

```

:::danger
While the compiler will allow you to use a class other than the owning class as the first parameter in `SynchedEntityData#defineId()`, this can and will lead to hard-to-debug issues and as such should be avoided.
Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer the wording used in the old entity networking page. This is not a case of "avoid if possible", it's a case of "seriously, don't do it, it WILL break".
I would also add a note that this includes adding data accessors via mixin to the technically correct class.

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.

Here's the requested feedback. Still a few WIP sections, so I'm wondering why it was brought out of draft early.

docs/blocks/index.md Outdated Show resolved Hide resolved
@@ -120,6 +120,7 @@ graph TD;
PlayerEvent-->CanPlayerSleepEvent;

class Event,BlockEvent,EntityEvent,LivingEvent,PlayerEvent red;
class BlockDropsEvent,CanPlayerSleepEvent blue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this in an entity docs PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly because this is the first PR that really makes use of Mermaid diagrams. I adopted the style guide of red -> abstract, blue -> non-abstract, and applied it to the only other diagram we had while I was at it.

docs/entities/attributes.md Show resolved Hide resolved
docs/entities/attributes.md Outdated Show resolved Hide resolved
docs/entities/attributes.md Outdated Show resolved Hide resolved
docs/entities/livingentity.md Outdated Show resolved Hide resolved
Comment on lines +156 to +158
Natural spawning is performed every tick for entities where `MobCategory#isFriendly()` is true, and every 400 ticks (\= 20 seconds) for entities where `MobCategory#isFriendly()` is false. If `MobCategory#isPersistent()` returns true, this process additionally also happens on chunk generation.

For each chunk and mob category, it is checked whether there are less than `MobCategory#getMaxInstancesPerChunk() * loadedChunks / 289` in the world. Additionally, for each chunk, it is required that there are less than `MobCategory#getMaxInstancesPerChunk()` entities of that `MobCategory` near at least one player (near means that the distance between mob and player \<\= 128) for spawning of that `MobCategory` to occur.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be best to frame this with an example to get the actual numbers for each.

Comment on lines +241 to +255
:::info
This section is a work in progress.
:::

### Brains

:::info
This section is a work in progress.
:::

### Navigation

:::info
This section is a work in progress.
:::
Copy link
Contributor

Choose a reason for hiding this comment

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

Work in progress?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and these sections will not be covered by the PR due to out-of-scope (and also because I do not have the energy to read into these systems at present).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just remove the sections for now and stick them in an issue if they're not already.

docs/entities/renderer.md Outdated Show resolved Hide resolved
}
```

### Adding a Layer Definition to an Entity
Copy link
Contributor

Choose a reason for hiding this comment

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

As Xfact mentions, layer definitions are not 'added' to an entity. Instead, the registered ModelLayerLocation is provided to the EntityModelSet to construct the ModelPart used to render the entity model. The below methods are for entity layers such as armor.

@neoforged-automation neoforged-automation bot added the needs rebase This Pull Request needs to be rebased before being merged label Jan 10, 2025
@neoforged-automation
Copy link

@IchHabeHunger54, this pull request has conflicts, please resolve them for this PR to move forward.

# Conflicts:
#	docs/resources/client/models/bakedmodel.md
#	docs/resources/server/conditions.md
@neoforged-automation neoforged-automation bot removed the needs rebase This Pull Request needs to be rebased before being merged label Jan 10, 2025
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) January 10, 2025 23:31 Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition Adding or rewriting information. large Major additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entities - Simply Complicated
5 participants