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

Add missing data to "supplyVanillaManipulators" methods for entities #2493

Merged

Conversation

Lignium
Copy link
Contributor

@Lignium Lignium commented Jan 28, 2020

This PR fully (with some exceptions) implements methods such as Entity#getValues​(), Entity#getKeys(), Entity#getContainers().

Some keys were simply not implemented (PRs implementing them were canceled due to the closing of the bleeding branch), which means that they are in principle not available in official Sponge implementations:

Just unimplemented data:

  • DamagingData (Keys.ATTACK_DAMAGE, Keys.DAMAGE_ENTITY_MAP)
  • ShatteringData (Keys.WILL_SHATTER)

And some just don't have a DataManipulator:

  • LlamaData (Keys.LLAMA_STRENGTH, Keys.LLAMA_VARIANT)

@ImMorpheus ImMorpheus added status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc labels Jan 31, 2020
Copy link
Contributor

@ImMorpheus ImMorpheus left a comment

Choose a reason for hiding this comment

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

Please don't add/remove newlines and move methods around. It adds noise to the diff, making it more difficult to review this PR.

Copy link
Member

@gabizou gabizou left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll try to getting to test some of this out with all entity types being spawned and calling the methods to verify what datas are available on them and what aren't, I might include that test as a pseudo integration test or unit test, depending on which one I'm able to mock better.

@ImMorpheus
Copy link
Contributor

Working as intended.
Tested with the following command executor:

for (EntityType type : Sponge.getRegistry().getAllOf(EntityType.class)) {
    try {
        ((Player) src).getWorld().createEntity(type, ((Player) src).getPosition()).getContainers();
    } catch (Exception e) {
        System.err.println("Error: " + e.getMessage());
    }
}

Output: (the following exceptions are not caused by these changes).

[Server thread/INFO]: Error: Cannot construct minecraft:complexpart please look to using entity types correctly!
[Server thread/INFO]: Error: There was an issue attempting to construct minecraft:fishinghook
[Server thread/INFO]: Error: Cannot construct minecraft:player please look to using entity types correctly!

@ImMorpheus ImMorpheus merged commit 67e25f9 into SpongePowered:stable-7 Mar 16, 2020
@Lignium Lignium deleted the fix/vanilla-data-manipulators branch April 5, 2020 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants