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.

Sign up for GitHub

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

Jump to bottom

MarkerVision: A Bomb Radar Rework #1203

Merged
merged 47 commits into from
Jan 22, 2024
Merged

MarkerVision: A Bomb Radar Rework #1203

merged 47 commits into from
Jan 22, 2024

Conversation

TimGoll
Copy link
Member

@TimGoll TimGoll commented Dec 14, 2023 •
edited
Loading

Added a new markerVision system that is similar to targetID. It handles wallhack and UI. It supports teamchange as well

https://www.youtube.com/watch?v=hdo0waT3cOw

@TimGoll
Copy link
Member Author

TimGoll commented Dec 14, 2023

I probably have to handle players being added to / removed from the team as well

@Histalek Histalek added the type/rework Big changes or overhaul of an existing feature label Dec 15, 2023
@TimGoll TimGoll marked this pull request as ready for review December 18, 2023 17:41
Histalek
Histalek requested changes Dec 19, 2023
View reviewed changes
Copy link
Member

@Histalek Histalek left a comment

Choose a reason for hiding this comment

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

Not a full review yet, IMHO this should be a draft.

i wanted to Veto calling this bombvision or radarvision.

Neither of them properly reflect what exactly this is and it is confusing as hell. Even more so as both names are still used throughout the PR.

Naming a library after a builtin item (that does not even use this) is beyond confusing.

The name for this i know from other games would be along the lines of "POI hints" (POI = Point of Interest) maybe we can find a better descriptive word even, but naming this after a specific item/equipment is out of the question for me.

gamemodes/terrortown/gamemode/server/sv_gamemsg.lua Outdated Show resolved Hide resolved
gamemodes/terrortown/entities/entities/ttt_radio.lua Outdated Show resolved Hide resolved
@TimGoll
Copy link
Member Author

TimGoll commented Dec 19, 2023

Not a full review yet, IMHO this should be a draft.

I don't get why this should be a draft as I'm more or less done with this

@Histalek
Copy link
Member

Not a full review yet, IMHO this should be a draft.

I don't get why this should be a draft as I'm more or less done with this

"Even more so as both names are still used throughout the PR." -> the PR itself does not refer to its introduced library in a consistent way which seems to be because of a recent change which in turn means it's still actively worked on -> should be a draft

@EntranceJew
Copy link
Contributor

fwiw i think POI / InterestPoint would be a decent name for it

@Spanospy
Copy link
Contributor

i wanted to Veto calling this bombvision or radarvision.

Neither of them properly reflect what exactly this is and it is confusing as hell. Even more so as both names are still used throughout the PR.

Seconding. While I think radarvision is a better name than bombvision, it can still be confused with the radar item that is completely unrelated to this HUD feature.

HUD Markers might be a better name?

@TimGoll TimGoll requested a review from Histalek January 11, 2024 10:49
@TimGoll
Copy link
Member Author

TimGoll commented Jan 11, 2024

Is there any input on this PR?

@Histalek
Copy link
Member

Is there any input on this PR?

Sorry i'll probably review it this weekend.

Until then i'll try one last time to propose another name. If that doesn't find support i'll just shut up about it.

Firstly i'll try to explain why i don't particularly like markerVision as a name:

  • Vision imo implies something that affects the whole vision of a player (like nightvision, thermalvision etc.) which this certainly does not do. This simply places pointers (or markers :D )
  • marker i would have liked this as a standalone name, but we also have a Marker role and this would just lead to unnecessary confusion.

My proposal therefore would be either waypoint or signpost.

  • waypoint is rather straightforward i think, a bit plain but maybe that's for the better
  • signpost a bit more corny, but would convey that this isn't only a simple pointer but can have more information (like distance etc.) attached

@NovaDiablox
Copy link
Contributor

How about ("entesp" = entity + esp)?

@TimGoll
Copy link
Member Author

TimGoll commented Jan 15, 2024

How about ("entesp" = entity + esp)?

We don't want to use abbreviations, but thanks for the suggestion!

@saibotk
Copy link
Member

saibotk commented Jan 15, 2024

What about entityMarks or just marks?

@TimGoll
Copy link
Member Author

TimGoll commented Jan 16, 2024

What about entityMarks or just marks?

We already have a marks module

@ZenBre4ker
Copy link
Contributor

What about entityMarks or just marks?

We already have a marks module

Maybe integrate it into the marks module?
The marks module currently only optimizes the halo library. Maybe it has some common ground?

ZenBre4ker
ZenBre4ker approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@ZenBre4ker ZenBre4ker left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Besides the name, the code looks fine. I also dont have a better idea. 😄

@TimGoll
Copy link
Member Author

TimGoll commented Jan 19, 2024

What about entityMarks or just marks?

We already have a marks module

Maybe integrate it into the marks module? The marks module currently only optimizes the halo library. Maybe it has some common ground?

nah, this has nothing to to with the marks module. The marks module just adds a colored stencil that is rendered on top, this adds networking, role change tracking, death tracking, icons, text and much more. Imho, they are not related

Histalek
Histalek requested changes Jan 20, 2024
View reviewed changes
Copy link
Member

@Histalek Histalek left a comment

Choose a reason for hiding this comment

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

a few doc suggestions and a small nit. Otherwise looks good

lua/ttt2/libraries/marker_vision.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/marker_vision.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/marker_vision.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/marker_vision.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/marker_vision.lua Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
gamemodes/terrortown/gamemode/client/cl_hud_manager.lua Outdated Show resolved Hide resolved
lua/ttt2/libraries/marker_vision.lua Outdated Show resolved Hide resolved
@TimGoll
Copy link
Member Author

TimGoll commented Jan 21, 2024

Everything should be done @Histalek and @saibotk

@TimGoll TimGoll mentioned this pull request Jan 21, 2024
Closed
@TimGoll TimGoll merged commit 71f24b6 into master Jan 22, 2024
3 checks passed
@TimGoll TimGoll deleted the bomb-vision branch January 22, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saibotk saibotk saibotk approved these changes

@Histalek Histalek Histalek approved these changes

@ZenBre4ker ZenBre4ker ZenBre4ker approved these changes

Assignees
No one assigned
Labels
type/rework Big changes or overhaul of an existing feature
Projects
None yet
Milestone
v0.13.0b
Development

Successfully merging this pull request may close these issues.

[Bomb Wallhack Renderer] Rework
7 participants
@TimGoll @Histalek @EntranceJew @Spanospy @ZenBre4ker @NovaDiablox @saibotk