Add CVar "mp_plant_c4_anywhere_delay" + API member. #694
Conversation
I liked the way you added a delay for planting the bomb as it helps on not planting when you just spawn. In my opinion, is best to make a signal to the player that he can plant the bomb right where he's standing. Also, on the API, I think that a member to let any player forced by the mod to plant the bomb like |
|
Yeah I have a first version using the signals too, because like you I also think the "read blink" should be used to tell the user "he can plant"! I basically see those facts: At the end, I think this decision might need a poll, or, at least opinion of other people and the more who will be "on a side" will be decisive! But right now, I think I should go back to the "blink code" like yours, and I could even remove the new message with that, which is not supported by client's translations due to localizations by the way, and instead, we will keep "#C4_Plant_At_Bomb_Spot" when feature not yet usable. Yeah the API member per-client (like specific setting for admins, etc.) is a very cool thing, and most of the features of the game should have it! |
|
So, "thumbs up" are cool, but still waiting for other people opinion via communication! |
|
@StevenKal If you need to "hurry up" I can recommend taking advantage of the "rich" possibilities of open source, as well as the possibilities of GitHub. I cannot do these repositories more intensively because I do not have enough free time. It may seem that the solution to the problem would be to give access rights to the repository to other "understanding" and "active" people, but the issue of trust remains an open one. I don't want to rush into a decision, and I also understand that people still have an interest in providing new functionality for GoldSrc. One of options for speeding up progress is to help with a code review. |
|
I was basically refering (at least here), to the people (overall) not saying their opinion about "that I am asking", this is not much complicated to say something like "When I can plant anywhere, "I prefer the C4 icon blinks red" or "I prefer the C4 icon does not blink red""!! About the "other kind of hurry up" (like you devs merge PRs a bit faster), yeah obviously a few more competent & serious people having some rights could help at speed up for sure, but as you said, the trust problem is there! Besides I do not think there are a lot of people who deserve such access. But I am pretty sure you usually come almost every day some minuts to look what is new in the Re* projects. The other problem as "being slow to merge", is that people usually do "small PRs per thing", not global ones containing a bunch of changes, so for example with the few francoromaniello's ReHLDS hooks PRs send one by one, if once he submits a PR, you will approve it almost directly you saw it (if you not against, and code O.K., and the "Actions" compiled it with success), and, before he works on his second hook PR, he could think to press "fetch & merge" on his ReHLDS forked repo (& should be informed his forks is outdated), and have a new branch for his second PR updated with actual dreamstalker's ReHLDS repo, so, when he will work & submit his second hook PR, you will not have to ask him "please update your code because I have added your previous hook PR too late and so your current code is out of date", and he will not have to waste his time to do this, neither do you if in case you do it yourself. About the "Public Releases", I personnaly think there is no need to do them at every commit (as you have to edit changes & contributors), except if this last is important (critical bug fix or new important changes/additions), each "new public release" can wait 1 month or a bit more, not much dramatic. So in overall you have to do them when there are a specific amount of changes and this worth to release a new one. And this is likely like you do apparently, so, good! However, I totally understand people have a life, besides, in your case you have to deal with the 3 Re* repository, which is some "charge" and not nothing! However, I can not lie you, I took last week, some pleasure to have fun at modifying it on a private repository, since I discovered this very useful "Actions" tab can automatically compile binaries for those Re* binaries, this is very nice of you devs to make available this to us! |
A large PR requires more time to review. Please take this into account. Not every solution may be a success, and it also takes time to discuss it all. I think this kind of discussion should be made in the discussion section (we will add it soon) and not in "some PR or issue". |
|
Thanks for your explanations! For the #1, I understand this, but most of those server owners using Re* binaries download them via links from here usually (as on dev-cs.ru as example, links point to here...), so people usually take a look at "how is GitHub", despite a lot skip that, but if they take some time, they can see the main/official development (requests, etc.) is made here, and nowhere else. Yeah I agree for the last, such section will be nice, for suggestions, etc., since the "Issues" one is not really made for this. |
Changes: Add new member "m_flPlantC4AnywhereDelay". Update function "CCSPlayer::JoinTeam" in order to drop the C4 when we move a player to spectator (fix issue rehlds#498). Update functions "CCSPlayer::DropShield" & "CCSPlayer::DropPlayerItem" by adding return value of the entity created, and provided by ReGameDLL_CS (not the case for the legit CS binary).
Changes: Add new member "m_flPlantC4AnywhereDelay". Update functions "CCSPlayer::DropShield" & "CCSPlayer::DropPlayerItem" by adding return value of the entity created, and provided by ReGameDLL_CS (not the case for the legit CS binary).
Changes reasons: No need to have those "*_api" functions for standard functions which do not have a different code/format, this is useless. Besides, passing the "real function used internally by the game" into the list of the structure "ReGameFuncs_t" will allow to retrieve the "real function address". So, we could "memhack it" and make a hook of the "real function", which is the internal one we want on a hook, not the API one which will only be called when we trigger those API functions. To finish, this ensures reliability of the function address, and, no need to search it via symbol or signature of bytes where the last can easily be broken. -> void *pfnAddMultiDamage = (void *)g_pReGameDLLCSFunctions->AddMultiDamage;
Changes reasons: No need to have those "*_api" functions for standard functions which do not have a different code/format, this is useless. Besides, passing the "real function used internally by the game" into the list of the structure "ReGameFuncs_t" will allow to retrieve the "real function address". So, we could "memhack it" and make a hook of the "real function", which is the internal one we want on a hook, not the API one which will only be called when we trigger those API functions. To finish, this ensures reliability of the function address, and, no need to search it via symbol or signature of bytes where the last can easily be broken. -> void *pfnAddMultiDamage = (void *)g_pReGameDLLCSFunctions->AddMultiDamage;
|
I finally restored the original code I had with the "blink code" & without the new message. But about the CVar & member name, I am wondering if it would not be better to name it like "mp_c4_plant_anywhere_delay", in order to start with the "mp_c4*" prefix and list it right before "mp_c4timer", and, especially if one day we add CVars like "mp_c4_frags_planter" & "mp_c4_frags_defuser" (change the "+3") & "mp_c4_frags_kills" (this last will call "g_pGameRules->PlayerKilled" to count the frags the explosion did)... |
|
Please do not mix code refactoring and introduction of new functionality in one PR. This is getting too complicated for code review. I recommend putting the refactoring into a separate PR. In the future, if you need to roll back changes, you will have to roll back both the new functionality and the refactoring.
They were simply copied (like a lot of things) from the existing implementation in CS:GO |
SergeyShorokhov
added
Priority: 🕒 low
|
Well, the "files changes" tab displays all of that has been modified, from that point this is not complicated (at least my point of view). And you, the day you will merge it into 1 commit, you could add like "+ API fixes #498*" to the title. But I will try in the future to do 1 PR per thing as you requested. |
|
In general, why this functionality? I don't see any useful uses, even with the change in gameplay with the bomb. |
|
Hum, let's say "for fun"! This is not much "worse" than some other CVars like "mp_buy_anywhere", "mp_infinite_*" as example. The only one problem I see is for the information of the CTs, a message at the new round telling them that feature is enabled will be welcomed, but a small plugin of just of few lines can be coded in conjunction of this, if needed. But I am a bit against adding such in the ReGameDLL_CS (a public message), knowing there is no localizations for the clients I think we should avoid it, except for some private admins stuff (game commands, etc.), that's all. To finish, if the feature is "there", & disabled by default, but the admins are free to enable it or not, like some other things which are available but they do not use, or they do, depending on their choices! Hehe! But I am just hesitating at renaming the CVar like I said above (mp_c4_plant_anywhere_delay), what the people is thinking about it? PS: Can you assign me on my other PR #689, I have a minor change to make, thanks! |
|
It would be better to create a more flexible solution, need to create 2 CVars instead of a combined one.
I plan to close this PR, and do everything in a single similar one (or create another one for the new CVar) |
| return true; | ||
| } | ||
|
|
||
| float CHalfLifeMultiplay::GetPlantC4AnywhereDelay(CBasePlayer *pPlayer) |
There was a problem hiding this comment.
I suppose the function under this is not appropriate here. CSPlayer->* would be enough.
| #ifdef REGAMEDLL_ADD | ||
| if(m_bHasC4 && IsAlive() && (m_iTeam == TERRORIST || m_iTeam == CT)) | ||
| { | ||
| float flPlantC4AnywhereDelay = CSGameRules()->GetPlantC4AnywhereDelay(this); |
There was a problem hiding this comment.
Here, it would be better to just refer to CSPlayer()->m_flPlantC4AnywhereDelay
Or do you see something better and more useful in using the function? Like creating a hook for it and using it in ReAPI. I don't see the benefit of creating a function, because it will be the same Think().
|
closed by #692 |
This comment has been minimized.
This comment has been minimized.
I don't agree with you or I misunderstood you. Below is the code that you can control for each individual client. set_member(index, m_bPlantC4Anywhere, <boolean>)
At this point, I've taken a simpler route for this kind of functionality, which is unlikely to resonate widely in use.
And that's great! We can help each other learn different nuances of this difficult thing without getting into a conflict. :)
You have a great idea. I suppose it would be worth discussing separately in another Issue. If you do that, you need to refactor many parts.
The final decision cannot be made by me anyway. This is an open product, which is created by many people (so I wish). And I just don't have enough skilfulness to compete in some decisions (for their implementation). However, at this point, I make the simplest decision based on its relevance. In any case, we can always expand and fix the meager implementation as well as the API, thanks to cooperation.
My subjective perception may differ greatly. Moreover, I do not claim to be "the smartest one here. I could be wrong :). Please worry less about such trivialities.
I would be happy to bring more understanding people into the discussion of your endeavors. However, I don't guarantee that they will come and they are still interested in doing this. (But, to get their attention, it would be better to localize what we would ask them to ponder. (there are too many words - For the next few times, let's try not to get bogged down in writing a memoir, but rather provide dry information to save time for anyone who is going to read it. A good tone would be to have a cold-blooded conversation of a substantive nature. For such long posts, it would also be good to format the right parts of the code with Markdown. Moreover, for the accessibility of your text and thought to a wider audience, it would be better to write more concisely, using more common words. As far as I understand it, it would be easier to have conversations that way. |
I think you misunderstood me for this, or do not see the advantage of it!
Thanks! But I will work on this later, this is just "something I have in mind in a TO DO list", because I have other things to do, but sometimes I like to change a bit & do other things (like submitting PRs for those good projects), the brain usually likes! Doing redundant things everytime can be exhausting & boring from a moment, surprise ourselves sometimes, with new tasks can be good! For the rest, as I said on the "EDIT" part in the end, all is O.K. now (I could even remove some parts of the topic, but I hidden now as "resolved"), your idea of having a separate delay for the C4 is good, the problem is that I misunderstood you & I though you wanted to split "the C4 anywhere delay feature I made" into 2 CVars (which I considered stupid because an unique one is simpler & avoid "adding 1 more CVar...," & me sometimes I enrage myself a little bit quicker on this period, sorry), and since this was on the "anywhere topic", while the "mp_plant_c4_delay" was in fact, for a global feature being independant, which is, as I said on #698, a good idea. About the C4 delay on the other topic, I quickly made a draft code yesterday at the evening, I will test it later, but I have added a new message for this, & also changed the "m_bPlantC4Anywhere" to work like I originaly wanted, since you did not pushed yet "an official release" we can change this, & please do not update AMXX's ReAPI module with this member till we did not pushed the C4 delay feature. Also do not push an official RG_CS release for now. About my writting style, I will try to simplify texts, but you know this is stronger than me to write a lot in order to give details, hard to reduce my habit regarding this, besides since my main PC is on XP, the extras buttons on the top-right (quote, bold, etc.), do not work, I will have to list the "codes of them" in a .txt file in order to use them because I do not recall all, since this is not "bbcode style" but other codes. |
Related to #683.
Add CVar "mp_plant_c4_anywhere_delay" + API member CCSPlayer's "m_flPlantC4AnywhereDelay" to allow per-client setting.
I made work the CVar to support a delay (or none when set to "-1") from the round start, because this is a bit abused to have that feature active from beginning! Like teams spawns then 5 seconds later "The bomb has been planted!".
Then I have also added a message informing the player who wants to plant outside from a bomb site that "he has to wait * second(s)", but the problem with that is there is no localization string (#*) on the client (so no game translations), so this is discutable, despite I think this worth to inform him about the feature.
So maybe we can discuss about it if you devs are against or not, or, what the overall player base is thinking.
About API member addition:
The new member CCSPlayer's "m_flPlantC4AnywhereDelay" has "-2.0" as defaut "unconsidered value", this is in order to allow per-client support of "restriction" (if set to "0") instead of redirecting in such case to the CVar. The only workaround to support this kind of feature is to set the CVar to 0 & set per-client settings via the member, so it is easier the way I did.
Also, I have updated ReGameDLL_CS's API minor because any addition to API even in the class "CCSEntity" worth to increment this (to know the new member requires a specific API version of ReGameDLL_CS, despite the "commits number" could be used as workaround for such "tiny" thing).
Extra notes:
Also added "EXPORT" label to 2 functions of the class "CHalfLifeMultiplay" in order to allow easy retrieval of the function address via symbol under Windows (like under Linux).
I could also make them "virtual" but I prefered to do not in order to avoid breaking already existing virtual IDs listing ("HasRoundTimeExpired" + "IsBombPlanted" being at the suite...) & AMXX's ReAPI module using full CSSDK (and not only the "pure public ReAPI interfaces" like my own AMX's ReAPI module).
The kind of per-client API member could be added for a lot of other features (mp_buytime, mp_buy_anywhere, mp_forcerespawn, mp_freeforall, etc.), I basically have plans to do it because this allows customization per-client & so interesting, like if you want to enable FFA for only 1 client (like I have this on my public classic server, I can TK my teammates (the dumb ones!) this counts as frag! Hehehehehe!).
PS: I did not see someone already pushed similar PR here like 1 hour before, and I had mine ready from yesterday, but mine is more "advanced" and does not deal with signals (not sure if it is the best but at least client carrying the C4 can know if he is on a bomb site... as indication, and this is also that Maxi605 said so I guess no red blink icon under CS:GO right? I can not check I do not have the game). Feel free to choose which one you want! Or, extend the other one with the features of mine (as the API member).