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 features specific to Shulkers, ShulkerBullets and ShulkerBoxes. #1903

Merged
merged 4 commits into from Mar 16, 2019

Conversation

RedNesto
Copy link
Contributor

@RedNesto RedNesto commented Aug 31, 2018

SpongeAPI | SpongeCommon

This PR adds everything I found for Shulkers and its derived products.

I only have a doubt about ShulkerBullet#getDirection, as it may be a DirectionalData.

Reviews are welcome.

Test Plugin

Fixes #1901

Also added EntityTargetingProjectile to abstract ShulkerBullet targeting.
@RedNesto RedNesto requested a review from gabizou as a code owner August 31, 2018 14:27
@RedNesto
Copy link
Contributor Author

~qa

@XakepSDK
Copy link

Links are broken here and in common

*
* @return the direction of this bullet.
*/
Direction getDirection();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be default implemented.

Copy link
Contributor Author

@RedNesto RedNesto Aug 31, 2018

Choose a reason for hiding this comment

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

What should I put in the default implementation? There is no data for it in Entity.
And I'm wondering if it would be better to expose the Direction through the Data API instead of a getter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought you already exposed DirectionalData for the bullet as well

Copy link
Member

Choose a reason for hiding this comment

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

Can very well expose directional data on the bullet.

*
* @see TargetedEntityData#value
*/
public static final Key<OptionalValue<Entity>> TARGETED_ENTITY = DummyObjectProvider.createExtendedFor(Key.class,"TARGETED_ENTITY");
Copy link
Member

Choose a reason for hiding this comment

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

Why is this an optional value? If there's no targeted entity, then the value shouldn't exist. Inversely, the value should only be returned as a valid value if there's a targeted entity (so you can support removing the targeted entity).

*
* @return the direction of this bullet.
*/
Direction getDirection();
Copy link
Member

Choose a reason for hiding this comment

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

Can very well expose directional data on the bullet.

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.

Leaving a decision for @Cybermaxke to make about targeted entity pointing to Entity references. Otherwise, the PR looks fine.

src/main/java/org/spongepowered/api/data/key/Keys.java Outdated Show resolved Hide resolved
@ImMorpheus ImMorpheus merged commit 7a77b95 into SpongePowered:stable-7 Mar 16, 2019
gabizou's TODO List automation moved this from Acknowledged to Done Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants