New shot handling backbone #1013
Conversation
Conflicts: src/main/java/com/flansmod/common/driveables/EntityDriveable.java src/main/java/com/flansmod/common/driveables/EntitySeat.java
|
I have conducted a few test: The GunTrail issue is not related to my changes in any way, in fact it exists for a very long time. For the grenade launcher: this bug also existed before my changes. |
ChrisLane
left a comment
ChrisLane
left a comment
There was a problem hiding this comment.
I've skimmed through the code and there are some cleanup changes to be made. Please act on the comments that I've made, some apply to many places in the changes but the comments only appear once.
I can see that you've added a significant amount of TODOs. If you could try to resolve as many of the TODOs as possible, especially the ones related to your changes, that would be great.
I'll be in-game testing when I get the time.
src/main/java/com/flansmod/apocalypse/common/entity/EntityFlansModShooter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flansmod/common/guns/raytracing/FlansModRaytracer.java
Show resolved
Hide resolved
src/main/java/com/flansmod/common/network/PacketBulletTrail.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: Liruxo <46451644+Liruxo@users.noreply.github.com>
|
I see some of my recent commits have come through as co-authored by you. Please merge next time to avoid confusing the commit tree. |
|
Thats weird, because i did merge. Well i try to avoid it but i am unsure what caused GitHub to add me as Co-Author |
|
I now have implemented most of your suggestions and marked the conversation as resolved. The remaining conservations all have an comment and are waiting for your response/approval/critic |
|
Merged and built recently. |
|
Running it with gradle runClient on Linux = no issues. |
|
I have now fixed numerous issues related to unloading/loading EntityBullet's, their trajectory, client/server desynchronization, etc. I am now finished as everything i tested related to my changes works as intended.
|
|
@rolylolypoly Well as you said i dont have any problems with this gradle build on linux, so i cant help you with windows problems. Anyways this sounds very suspicious since gradle, git and java should work the same on any operating system |
jamioflan
left a comment
jamioflan
left a comment
There was a problem hiding this comment.
Happy to take this as is. The number of outstanding TODOs is no worse than what's already in the source, and a review this big is likely to take forever unless we get it in.
|
|
||
| lookVector.scale(500.0f); | ||
| FireableGun fireableGun = new FireableGun(gunType, gunType.damage, gunType.bulletSpread, gunType.bulletSpeed); | ||
| //TODO unchecked cast, grenades will cause a crash (currently no vehicle with this feature exists) |
There was a problem hiding this comment.
Can't we just catch it, error and return?
There was a problem hiding this comment.
I have inserted a casting check for good coding practices, but i plan on reworking grenades anyways which would completely resolve all problems related to them
| //TODO unchecked cast, grenades wont work (currently no vehicle with this feature exists) | ||
| FiredShot shot = new FiredShot(fireableGun, (BulletType) bulletItem.type, this, (EntityPlayerMP) getDriver()); | ||
|
|
||
| ShootBulletHandler handler = (Boolean isExtraBullet) -> |
There was a problem hiding this comment.
Could this name be more descriptive? ShotFiredSuccessHandler or something? It doesn't say which bit of shooting is being handled
There was a problem hiding this comment.
Well it is just a handler called every time when a bullet is fired, so ShotFiredSuccessHandler doesn't really make sense to me. I have added a description in the interface, this may help understanding the function of the interface
| { | ||
| ShootableType shootableType = ((ItemShootable)bulletStack.getItem()).type; | ||
| FireableGun fireableGun = new FireableGun(gunType, gunType.getDamage(stack), gunType.getSpread(stack), gunType.getBulletSpeed(stack)); | ||
| //TODO unchecked cast, grenades will cause an error |
There was a problem hiding this comment.
Mechas definitely can shoot grenades. I'm happy to put this PR in with some TODOs in though as we need to get some progress on it
|
Also @Liruxo, can you send an e-mail to admin@flansmod.com? I'd like to send you a Discord invite |
4 participants
First of all: I am sorry, this is quite a big pull request because i am unable to split this into multiple PRs due to the nature of this project
Second this also fixes multiple issues, because they are all related to the old shooting code
The Content of this PR:
-The raytrace is now always performed on the server side
Problems:
-Mechs currently cant use grenade launching guns, because this would require a rewrite of the grenade related classes
-There may be some "unsmooth" playing experience due to packet delay over the internet, which is a problem since now the client needs to wait for the server to make hit related effects and sounds
-GunTrails are not visible when standing to close to the origin of the shot
Questions:
-To counter the second Problem i have implemented a Queue (found in the PlayerData class) to delay shoots until they should be shot. This however results in constantly resetting the magazine on the client side, because for the player all this shots were actually fired with the correct timings. The current solution is to skip ticks on the server side and just trust that the client is using the correct timings.
The question is how this should be handled
-GunTrails are currently visible to all players, which wasnt possible before because no Packet to realize this existed. Should GunTrails be visible to all players?