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

C4 Rework #1739

Merged
merged 27 commits into from
Jan 29, 2025
Merged

C4 Rework #1739

merged 27 commits into from
Jan 29, 2025

Conversation

MrXonte
Copy link
Contributor

@MrXonte MrXonte commented Jan 23, 2025

Now scales C4 damage with radius and gives consistent damage through walls regardless of line of sight.
Relates to #1729 and #1730 although the calculation for proper Killzone is not yet included, but the safe and killzones are now 100% correct.

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 23, 2025

im not sure what im doing wrong for the total history to show up, but I sycnched my fork before PR #1738 and it showed up, now I made a branch and its still all there

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 23, 2025

Also, whats wrong with the style check? Cant figure out what the problem here is
image

@TimGoll
Copy link
Member

TimGoll commented Jan 23, 2025

im not sure what im doing wrong for the total history to show up, but I sycnched my fork before PR #1738 and it showed up, now I made a branch and its still all there

If you make a new branch AFTER the commits were added, the new branch won't change anything.

I barely work with forks, so my knowledge is spotty. But you probably not only need to sync to the host, but also rebase it, so your commit history is removed in favor of ours (we squash PRs, so your commits are not in our repository)

@TimGoll
Copy link
Member

TimGoll commented Jan 23, 2025

Also, whats wrong with the style check? Cant figure out what the problem here is image

@Histalek looks like stylua crashed?

@MrXonte the crash might have happened because you removed the empty line at file end (which always should be there)

image

@Histalek
Copy link
Member

Also, whats wrong with the style check? Cant figure out what the problem here is image

@Histalek looks like stylua crashed?

@MrXonte the crash might have happened because you removed the empty line at file end (which always should be there)

image

that's not a crash and only stylua reporting a style issue (but that style issue is about non-printable characters, which makes the output you see kinda useless)

and yes you're spot on with the missing newline at the end of the file

lua/ttt2/libraries/game_effects.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/game_effects.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/game_effects.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/game_effects.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/game_effects.lua Outdated Show resolved Hide resolved
@MrXonte MrXonte requested a review from TimGoll January 27, 2025 09:19
@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

If you change something, I'm available for another 30 minutes or so

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 27, 2025

If you change something, I'm available for another 30 minutes or so

if its ok with you, I'd set the Outer range to 1500? Heres a comparison:
image
image

or we set it to linear

@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

yeah, that looks good and it compensates the old LOS kill I think

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 27, 2025

image

Also, I'm not sure if this was different before, but C4 is not listed as inflictor. Do you know how it was before?

no idea how it was before, but im doing it the same way the old spheredamage did. It used self for the inflictor, and im parsing self as the inflictor into the new spheredamage, so it should be the same as before
OLD:
image
NEW:
image
image

increased outer radius to 1500
@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

no idea how it was before, but im doing it the same way the old spheredamage did. It used self for the inflictor, and im parsing self as the inflictor into the new spheredamage, so it should be the same as before

Yes I looked at the code as well. I'm confused. I guess it did not work before either because you pass self, which is the c4 ent, but not the c4 weapon.

could you try to pass ents.Create("weapon_ttt_c4") instead of self?

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 27, 2025

no idea how it was before, but im doing it the same way the old spheredamage did. It used self for the inflictor, and im parsing self as the inflictor into the new spheredamage, so it should be the same as before

Yes I looked at the code as well. I'm confused. I guess it did not work before either because you pass self, which is the c4 ent, but not the c4 weapon.

could you try to pass ents.Create("weapon_ttt_c4") instead of self?

it should also be possible to parse the entity instead of the weapon, but I'm gonna check later

@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

it should also be possible to parse the entity instead of the weapon

Actually I don't think so. Looking at the code here makes me believe this never worked:
image

@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

If my suggestion fixes it, could you please test it in an old TTT2 version (before your PR) to confirm it was broken there as well? If it was, could you then please add this to the changelog? Thank you!

@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

Also, I don't know if this should be part of this PR, but if you changed the kill/damage radius, you should probably also change the UI (marker vision) to this?

proper safe/damage/killzone UI
@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 27, 2025

Also, I don't know if this should be part of this PR, but if you changed the kill/damage radius, you should probably also change the UI (marker vision) to this?

I've included it now since i do think it should be part of the PR. if we are gonna change how it works, we should also make sure the display is (finally) correct

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 27, 2025

it should also be possible to parse the entity instead of the weapon

Actually I don't think so. Looking at the code here makes me believe this never worked: image

hm this complicates things, but i guess that just means we should finally expand this class to allow for entities to be displayed here? A well defined entity should include all the relevant data we want to display here since we just do a lot of back and forth to print out the entity printname/id anyways?

@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

hm this complicates things, but i guess that just means we should finally expand this class to allow for entities to be displayed here? A well defined entity should include all the relevant data we want to display here since we just do a lot of back and forth to print out the entity printname/id anyways?

I have to look into that. In theory that could be an easy change, but I'm not sure if this has any unwanted side effects.

I know I spent some time getting the boomerang to work because it is also a generic entity, not a weapon. I solved this, but I forgot how

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 27, 2025

hm this complicates things, but i guess that just means we should finally expand this class to allow for entities to be displayed here? A well defined entity should include all the relevant data we want to display here since we just do a lot of back and forth to print out the entity printname/id anyways?

I have to look into that. In theory that could be an easy change, but I'm not sure if this has any unwanted side effects.

I know I spent some time getting the boomerang to work because it is also a generic entity, not a weapon. I solved this, but I forgot how

if your entity has a .ScoreName then this is taken instead if its not a weapon, at least for the kill event.
image
In the corpse its worse, since it only allows for a weapon or nothing at all, but if you parsed an entity here it might also work (or break horribly)
image

@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

We have to make TTT3 that breaks compatibility with all that shit, that looks awful :D

I'm not sure what I changed for bodysearch. But I did something to support weapons such as the boomerang

Edit: Found the change: #1382

However, this only fixes missing icons. This doesn't address the fact that C4 isn't registered at all

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 27, 2025

If my suggestion fixes it, could you please test it in an old TTT2 version (before your PR) to confirm it was broken there as well? If it was, could you then please add this to the changelog? Thank you!

as the placer of the C4, I get the C4 note
image
but definitely no indication that C4 was the cause when it kills someone else
image

@TimGoll
Copy link
Member

TimGoll commented Jan 27, 2025

Thank you for investigating this!

Speaking of this: I hate that the C4 note is hardcoded in the TTT2 code base. I have to remove it from there and use the same pipeline that addons use.

@TimGoll
Copy link
Member

TimGoll commented Jan 28, 2025

In case you're waiting on me. I'm fine with this as soon as you test the ents.Create("weapon_ttt_c4") suggestion

@TimGoll
Copy link
Member

TimGoll commented Jan 29, 2025

this change actually fixed it:

image

For me this PR is done. If you want to investigate registering entites as inflictors, feel free to go ahead in a further PR

@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 29, 2025

In case you're waiting on me. I'm fine with this as soon as you test the ents.Create("weapon_ttt_c4") suggestion

i actually just didn't have time yet, but great that it seems to work now! :D (also Eieruhr C4 xD)

@TimGoll
Copy link
Member

TimGoll commented Jan 29, 2025

(also Eieruhr C4 xD)

:D That's for the PietSmiet easteregg pack

FYI, I've got some plans for the rework of the C4 UI I'm currently working on. This will trigger some reworks of the damage again. So don't be confused when I change the parameters again.

@TimGoll TimGoll merged commit a438ec6 into TTT-2:master Jan 29, 2025
4 checks passed
@MrXonte
Copy link
Contributor Author

MrXonte commented Jan 29, 2025

(also Eieruhr C4 xD)

:D That's for the PietSmiet easteregg pack

FYI, I've got some plans for the rework of the C4 UI I'm currently working on. This will trigger some reworks of the damage again. So don't be confused when I change the parameters again.

aahhhh i knew i knew it from somewhere xD

the MV UI elements or the use UI? 🤔 Also I think scaling damage with the fuze time would be great. Its a bit weird to only make it harder to defuse since you can just go away from it much more easily if the fuze is longer.

@TimGoll
Copy link
Member

TimGoll commented Jan 29, 2025

See #1734, the PR is about the UI and some light rebalancing

If you want to discuss my ideas, it is best to post them there

Histalek pushed a commit to WardenPotato/TTT2 that referenced this pull request Jan 31, 2025
Now scales C4 damage with radius and gives consistent damage through
walls regardless of line of sight.
Relates to TTT-2#1729 and TTT-2#1730 although the calculation for proper Killzone
is not yet included, but the safe and killzones are now 100% correct.

---------

Co-authored-by: Tim Goll <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants