Skip to content

Extensible handlers for entities #734

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

Open
wants to merge 6 commits into
base: version/main
Choose a base branch
from

Conversation

uecasm
Copy link
Contributor

@uecasm uecasm commented Apr 19, 2025

Closes discord request
Closes discord request

Changes proposed in this pull request

  • Moves the "Remove Filtered" button below the resource list to hopefully reduce misclicks.
  • Fixes some off-by-one errors when converting bounding boxes to AABBs (particularly for entity searches).
  • Adds "entity handlers", which similar to placement handlers are extensible by addon mods to hook into UI and placement behaviour, instead of relying on hard-coded checks.
    • As before, only certain entities will place in survival mode (assuming Minecolonies or another survival handler is installed).
    • Entities without any handler are not placeable in survival (but can still be placed in creative). When a handler is defined then the entity becomes placeable by default, unless the handler says otherwise.
    • Glow item frames now require themselves as resources, instead of being a free upgrade from normal item frames.
  • The scan tool entity list now shows some extra icons:
    • Up to four item icons showing the resource types that will be required in survival mode.
    • A red/green indicator showing whether this entity can be placed in survival mode. (This is somewhat irrelevant to Structurize alone but useful if there's any survival mod on top.)

image

Testing

  • Yes I tested this before submitting it.
  • I also did a multiplayer test.

Review please (should port)

Copy link
Member

@Nightenom Nightenom left a comment

Choose a reason for hiding this comment

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

will do full review later, just some quick things to consider

final List<? extends Entity> list = entity.level().getEntitiesOfClass(entity.getClass(), AABB.unitCubeFromLowerCorner(posInWorld));
for (Entity worldEntity : list)
{
if (worldEntity.position().equals(posInWorld))
Copy link
Member

Choose a reason for hiding this comment

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

This seems sus, are we considering same class same pos as same entities? what about their content/wouldn't it be better to use uuid to prevent dupes? (along with consideration of vanilla in world uuid deduplication)

Copy link
Contributor Author

@uecasm uecasm Apr 19, 2025

Choose a reason for hiding this comment

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

This is essentially what it did before, except it used an even larger range of blocks to search within (possibly to allow for mob wandering slightly?).

It can't use UUID because it's guaranteed that the entity in-world will have a different UUID from the entity in-blueprint.

Copy link
Member

Choose a reason for hiding this comment

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

note for myself to revisit this in testing

@uecasm uecasm marked this pull request as ready for review April 29, 2025 08:59
Copy link
Member

@Nightenom Nightenom left a comment

Choose a reason for hiding this comment

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

I hope you tested thoroughly, I personally use following (4 years old :D) entity blueprint along with one for paintings, both in also mirror settings. I want to do in-game testing myself, but not sure when will get to it

it's txt because github uploading.. before using rename to .blueprint
debug_entities.txt

@Override
public boolean canHandle(final Entity entity)
{
return entity instanceof Mob;
Copy link
Member

Choose a reason for hiding this comment

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

don't have ide open, what is lower livingEntity or mob in entity class hierarchy? iirc mob implements livingentity, if so why not instanceof livingEntity?
(i see it was in old code, just asking)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mob is derived from LivingEntity. There's probably no particular reason this couldn't be LivingEntity, but I was trying to keep previous behaviour, and that's what it did before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SpawnEggItem(EntityType<? extends Mob> p_43207_, ...

Actually this is probably why. It's technically only legal to create spawn eggs for Mobs.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, wondering if we want to resolve the rest of the set - ie. complement to Mob in LivingEntity set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any entity not handled by any entity handler will still get default behaviour: placeable in creative mode only, no resources listed, best guess at icon.

The only thing the Mob handler really does differently from default is to give it a resource list, which is kinda redundant since it can't be placed in survival anyway.

@Override
public boolean canPlace(final Entity entity, final boolean isCreative, final boolean isFancy)
{
return isCreative && !isFancy;
Copy link
Member

Choose a reason for hiding this comment

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

wonder that all Display types should be placed if isCreative, if mcol wants !isFancy then it should be there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also matches previous behaviour. But even for pure structurize it makes sense to me to keep them around only for "schematic paste" and not "constructed", because the purpose of these was to show notes in the preview that don't survive when built.

Copy link
Member

@Nightenom Nightenom Apr 30, 2025

Choose a reason for hiding this comment

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

I see that, for now fine, but later for in-blueprint notes we should rather create our extension of Diplay.Text that is backed using tags.
Remaining question: the other Display types? (btw I get you tried to keep up with old code, but since we're unifying logic here we could make it more proper right away)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we could extend support for those too.

final List<? extends Entity> list = entity.level().getEntitiesOfClass(entity.getClass(), AABB.unitCubeFromLowerCorner(posInWorld));
for (Entity worldEntity : list)
{
if (worldEntity.position().equals(posInWorld))
Copy link
Member

Choose a reason for hiding this comment

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

note for myself to revisit this in testing

@uecasm
Copy link
Contributor Author

uecasm commented Apr 30, 2025

testing

image

You can see most of the test blueprints I was using in the description screenshot. Mostly paintings and item frames, but I tried several other things too. Pictured here is your blueprint rotated twice, placed, and scan-boxed again, it looks ok AFAICT.

@uecasm uecasm requested a review from Nightenom May 28, 2025 09:33
Copy link
Contributor

@Raycoms Raycoms left a comment

Choose a reason for hiding this comment

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

This looks good. I think as a next step we should maybe look at better entity matching.
(I.e. not only same pos and type, but also same inventory) and then do a "replace" (break and pick up).

@Nightenom
Copy link
Member

Nightenom commented May 28, 2025

I've DMed you @uecasm, I'm generally fine with changes but I would like to test it myself on 1.21, so either you or I need to port this branch first, tell me whatever works best for you

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.

4 participants