ddnet icon indicating copy to clipboard operation
ddnet copied to clipboard

Add interact state to fix invisible lasers

Open ChillerDragon opened this issue 8 months ago • 14 comments

This is a ~~work in progress~~ refactor for projectile visibility and interact state to solve https://github.com/ddnet/ddnet/issues/9446 and similar issues. The core problem is that projectiles use the owner character to lookup state. But the owner might be dead or disconnected or switch ddrace teams and similar.

The proposed fix is to store all state needed to determine if a projectile can hit or be seen directly on the projectile. It still queries the full state every tick from the character to not change any old behavior even if it was weird. This can still be done in a feature refactor. When the owner disconnects it just keeps the old state instead of resetting to some unset state as it is doing right now.

~~The code is work in progress and just an idea for now. It still needs to be finished and polished. Because this is time consuming I would like to have some maintainer feedback before I put a lot of time into this refactor.~~

After 2 weeks of no feedback I fully finished and tested the pr. Works fine and is ready to be merged.

This pr does not fix grenades or id reuse: #10398, #9446 but lays some good foundation for it

This pr already fixes #10417, #9229

https://github.com/user-attachments/assets/067fb779-b8fc-494d-800a-b214e5a644d1

ChillerDragon avatar Jun 21 '25 10:06 ChillerDragon

This can still be done in a feature refactor.

Do you intend to do that refactor or are you just remarking that anything can be done by changing code?

heinrich5991 avatar Jul 12 '25 16:07 heinrich5991

Do you intend to do that refactor or are you just remarking that anything can be done by changing code?

Yes I plan to fix more once this is merged

ChillerDragon avatar Jul 12 '25 20:07 ChillerDragon

I would love to see some progress here! Anything specific blocking here? Or why has nobody reviewed it yet? Is the fix-changes-physics label too scary? I can also exclude the fix of #10417 and remove the physics change and keep it as just a visual fix for now if it helps the merge.

ChillerDragon avatar Jul 22 '25 06:07 ChillerDragon

The proposed fix is to store all state needed to determine if a projectile can hit or be seen directly on the projectile. It still queries the full state every tick from the character to not change any old behavior even if it was weird. […] When the owner disconnects it just keeps the old state instead of resetting to some unset state as it is doing right now.

This sounds like a really good idea, I like it. :) It makes the code more explicit about its intention. :)

I'm unsure about the name InteractState, but if we find a better name for it, we can just search-replace it. :)

heinrich5991 avatar Jul 29 '25 15:07 heinrich5991

It looks sane, it feels sane, hard to make sure it didn't change anything. At least a cursory glance suggests it didn't. @heinrich5991 does this need a teehistorian bump, just in case if it changed something accidentally?

Learath2 avatar Jul 29 '25 17:07 Learath2

That would be a question for @Zwelf or @Patiga.

heinrich5991 avatar Jul 29 '25 17:07 heinrich5991

It changed something intentionally and does require a teehistorian bump! See https://github.com/ddnet/ddnet/pull/10399#issuecomment-3101236726

ChillerDragon avatar Jul 29 '25 17:07 ChillerDragon

What is the plan here? How do I get this merged?

ChillerDragon avatar Aug 24 '25 09:08 ChillerDragon

Any updates?

ChillerDragon avatar Sep 09 '25 16:09 ChillerDragon

Any update here? Can I do something to progress the merge?

ChillerDragon avatar Oct 27 '25 10:10 ChillerDragon

@Robyt3 can you take a look please? This is open for half a year now and I don't see why this is not merged yet

AssassinTee avatar Nov 10 '25 10:11 AssassinTee

github is hiding the reply box gg

image

https://github.com/ddnet/ddnet/pull/10399#discussion_r2514438269

The plan is to not maintain duplicated code. But have the new class as source of truth it should be called for everything. Its not cut out yet to keep the pr thinner while its stuck in review. This pr is only the start of the cleanup.

ChillerDragon avatar Nov 11 '25 14:11 ChillerDragon

Amazing, I have the same github bug.

heinrich5991 avatar Nov 11 '25 14:11 heinrich5991

Ah, nevermind, it's probably because it's an earlier comment thread.

heinrich5991 avatar Nov 11 '25 14:11 heinrich5991