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

Bow and arrow fixes #1098

Merged
merged 10 commits into from Jun 26, 2014

Conversation

Projects
None yet
5 participants
@Howaner
Contributor

Howaner commented Jun 16, 2014

  • Add bow charging animation.
  • Fix bow sounds.
  • Players in survival mode can't pick up the arrow, when the shooter was in creative-mode.

@Howaner Howaner changed the title from Bow changes to Bow Jun 16, 2014

@archshift

This comment has been minimized.

Show comment
Hide comment
@archshift

archshift Jun 16, 2014

Contributor

Please, some description! What does your PR add? You should have a descriptive title and comment that explains what you actually did.

Contributor

archshift commented Jun 16, 2014

Please, some description! What does your PR add? You should have a descriptive title and comment that explains what you actually did.

Show outdated Hide outdated src/Items/ItemBow.h

@archshift archshift changed the title from Bow to Bow and arrow fixes Jun 17, 2014

Howaner added some commits Jun 17, 2014

@Howaner

This comment has been minimized.

Show comment
Hide comment
@Howaner

Howaner Jun 17, 2014

Contributor

Changed.

Contributor

Howaner commented Jun 17, 2014

Changed.

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft Jun 17, 2014

Member

I still don't understand why you're checking the m_PickupState for the arrow. It's already been checked by CanPickup, and there ARE arrows that can be picked even in creative. Vanilla doesn't create them, but obeys them when created using NBT editors.

Member

madmaxoft commented Jun 17, 2014

I still don't understand why you're checking the m_PickupState for the arrow. It's already been checked by CanPickup, and there ARE arrows that can be picked even in creative. Vanilla doesn't create them, but obeys them when created using NBT editors.

@Howaner

This comment has been minimized.

Show comment
Hide comment
@Howaner

Howaner Jun 17, 2014

Contributor

Only creative player can pickup arrows from other creative players.
But the arrow wouldn't add to the inventory.

Vanilla:
https://github.com/Bukkit/mc-dev/blob/master/net/minecraft/server/EntityArrow.java#L372

Contributor

Howaner commented Jun 17, 2014

Only creative player can pickup arrows from other creative players.
But the arrow wouldn't add to the inventory.

Vanilla:
https://github.com/Bukkit/mc-dev/blob/master/net/minecraft/server/EntityArrow.java#L372

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft Jun 17, 2014

Member

You shouldn't check the m_PickupState then, but the player instead, if they are in Creative. And add a comment along those lines to the source.

Member

madmaxoft commented Jun 17, 2014

You shouldn't check the m_PickupState then, but the player instead, if they are in Creative. And add a comment along those lines to the source.

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft Jun 25, 2014

Member

@Howaner could you fix the PR so that we can merge it in? Issue #1127 will conflict with this PR, so I'd prefer to have it merged already :)

Member

madmaxoft commented Jun 25, 2014

@Howaner could you fix the PR so that we can merge it in? Issue #1127 will conflict with this PR, so I'd prefer to have it merged already :)

Howaner added some commits Jun 26, 2014

@Howaner

This comment has been minimized.

Show comment
Hide comment
@Howaner

Howaner Jun 26, 2014

Contributor

done

Contributor

Howaner commented Jun 26, 2014

done

@madmaxoft madmaxoft merged commit bf32298 into cuberite:master Jun 26, 2014

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci The Travis CI build passed
Details
@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft Jun 26, 2014

Member

I merged the code, but I had to remove the BroadcastCollectPickup() call. It expects a cPickup, you can't simply cast a cArrowEntity to a cPickup. The code compiles, but it will crash.
In order to implement proper collection broadcast, you will need to change the BroadcastCollectPickup function: Rename it to BroadcastCollectEntity, make it take a cEntity & and the protocol will have to see what packet to send, based on the entity type (pickup / arrow / XP orb (?) / ...), if it even needs to distinguish between those. If not, even better, we can make "collectible creepers" :)

Member

madmaxoft commented Jun 26, 2014

I merged the code, but I had to remove the BroadcastCollectPickup() call. It expects a cPickup, you can't simply cast a cArrowEntity to a cPickup. The code compiles, but it will crash.
In order to implement proper collection broadcast, you will need to change the BroadcastCollectPickup function: Rename it to BroadcastCollectEntity, make it take a cEntity & and the protocol will have to see what packet to send, based on the entity type (pickup / arrow / XP orb (?) / ...), if it even needs to distinguish between those. If not, even better, we can make "collectible creepers" :)

@tigerw

This comment has been minimized.

Show comment
Hide comment
@tigerw

tigerw Jun 26, 2014

Member

But the arrow collection animation was working fine before with that call.

Member

tigerw commented Jun 26, 2014

But the arrow collection animation was working fine before with that call.

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft Jun 26, 2014

Member

That's just a coincidence that it worked for you, the C++ standard says it's an undefined behavior, so the program was allowed to start a nuclear war and it'd still be correct.

Member

madmaxoft commented Jun 26, 2014

That's just a coincidence that it worked for you, the C++ standard says it's an undefined behavior, so the program was allowed to start a nuclear war and it'd still be correct.

@worktycho

This comment has been minimized.

Show comment
Hide comment
@worktycho

worktycho Jun 26, 2014

Member

@tigerw working is a valid value of anything can happen.

Member

worktycho commented Jun 26, 2014

@tigerw working is a valid value of anything can happen.

@tigerw

This comment has been minimized.

Show comment
Hide comment
@tigerw

tigerw Jun 26, 2014

Member

Ah. I've always wondered about undefined behaviour and the law.

If the program, through undefined behaviour, commits a crime (blackmailing your neighbour's cat, for example), who is responsible? The standard makers? The compiler programmers? The programmer? The program as a legal entity? Is it a grey area?

If there is no punishment, who gives the authority for the program to do what it likes?

Member

tigerw commented Jun 26, 2014

Ah. I've always wondered about undefined behaviour and the law.

If the program, through undefined behaviour, commits a crime (blackmailing your neighbour's cat, for example), who is responsible? The standard makers? The compiler programmers? The programmer? The program as a legal entity? Is it a grey area?

If there is no punishment, who gives the authority for the program to do what it likes?

@worktycho

This comment has been minimized.

Show comment
Hide comment
@worktycho

worktycho Jun 26, 2014

Member

@tigerw probably the company/organisation responsible for creating the program. See the Toyota case. However it is a bit of a grey area as there haven't been many cases of programs committing crimes through failure.

Member

worktycho commented Jun 26, 2014

@tigerw probably the company/organisation responsible for creating the program. See the Toyota case. However it is a bit of a grey area as there haven't been many cases of programs committing crimes through failure.

madmaxoft added a commit that referenced this pull request Jun 27, 2014

Added generic entity-collecting.
Now any cEntity can be collected, not only cPickups.
This should help PR #1098.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment