-
Notifications
You must be signed in to change notification settings - Fork 219
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
Pubovl (including extras) #905
base: master
Are you sure you want to change the base?
Conversation
I suppose I should have removed the comments... |
* Removed useless comments. Rewritten a couple. * Modernized some code fragments. * Fixed indentation stuff (hopefully).
There, this is ready for review. |
* Removed `cg_multiTextureStrength` (unused). * Decreased `cg_textureStrength` by half. * Adjusted min/max of `cg_outlineStrength`.
@Conticop Could you get this updated with the latest master changes ? |
@yvt Ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Although it's marked as draft now, I'm going to submit a review anyway to explain why it cannot be merged in the current state:
The largest problem is already stated by @siecvi: In addition to fixing #807, the PR introduces multiple features irrelevant to the primary change, such as the outline (NotTodo 2-3) and texturing effects and even global sound volume control. The PR must be kept minimal and focus on a single logical change.
Another problem concerns code quality - bypassing abstraction, the use of global variables, and such. The existing code is bad as it is, but let's not make it even worse.
@@ -190,6 +190,7 @@ namespace spades { | |||
@field = CommandField(Manager, ui.chatHistory); | |||
field.Bounds = AABB2(winX, winY, winW, 30.f); | |||
field.Placeholder = _Tr("Client", "Chat Text"); | |||
field.MaxLength = 93; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant to the primary change.
Resources/Scripts/Gui/Preferences.as
Outdated
layouter.AddHeading(_Tr("Preferences", "OpenGL Effects")); | ||
layouter.AddToggleField(_Tr("Preferences", "Outlines"), "cg_outlines"); | ||
layouter.AddSliderField(_Tr("Preferences", "Outline Strength"), "cg_outlineStrength", 2, 5, 1, | ||
ConfigNumberFormatter(0, "px")); | ||
layouter.AddToggleField(_Tr("Preferences", "Textures"), "cg_textures"); | ||
layouter.AddToggleField(_Tr("Preferences", "Multi-Texture Mode"), "cg_multiTextures"); | ||
layouter.AddSliderField(_Tr("Preferences", "Texture Strength"), "cg_textureStrength", 0, 100, 1, | ||
ConfigNumberFormatter(0, "%")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outline (NotTodo 2-3) and texturing effects are clearly out of scope of this PR and should be removed.
namespace spades { | ||
namespace client { | ||
|
||
Client *Client::globalInstance = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't store Client *
in a global variable.
@@ -479,6 +481,10 @@ namespace spades { | |||
void LocalPlayerCreatedLineBlock(IntVector3, IntVector3) override; | |||
void LocalPlayerHurt(HurtType type, bool sourceGiven, Vector3 source) override; | |||
void LocalPlayerBuildError(BuildFailureReason reason) override; | |||
|
|||
static bool AreCheatsEnabled(); // 'cheats', i.e. spectator wallhack or player names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The name needs to be more descriptive. How is this different from
WallhackActive
? - Don't make this member function
static
@@ -479,6 +481,10 @@ namespace spades { | |||
void LocalPlayerCreatedLineBlock(IntVector3, IntVector3) override; | |||
void LocalPlayerHurt(HurtType type, bool sourceGiven, Vector3 source) override; | |||
void LocalPlayerBuildError(BuildFailureReason reason) override; | |||
|
|||
static bool AreCheatsEnabled(); // 'cheats', i.e. spectator wallhack or player names | |||
static bool WallhackActive(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Rename this to
IsWallhackActive
- Don't make this member function
static
// Update master volume control | ||
if (s_volume_previous != (int)s_volume) { | ||
// update the previous volume | ||
s_volume_previous = (int)s_volume; | ||
// compute the new dB level, where 27.71373379 ~ 10^(1/log(2)), and update | ||
// the master gain to it | ||
if ((int)s_volume == 0) { | ||
dBPrevious = 0; | ||
} else { | ||
dBPrevious = powf(27.71373379f, log(((float)s_volume) / 100.0f)); | ||
} | ||
al::qalListenerf(AL_GAIN, dBPrevious); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant to the primary change.
SPADES_SETTING(s_volume); | ||
extern int s_volume_previous = 100; // keep track of the "previous" volume so the dB isn't recomputed when unnecessary | ||
extern float dBPrevious = 1.0f; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant to the primary change.
SPADES_SETTING(s_volume); | ||
extern int s_volume_previous; | ||
extern float dBPrevious; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant to the primary change.
|
||
// Update master volume control | ||
if (s_volume_previous != (int)s_volume) { | ||
// update the previous volume | ||
s_volume_previous = (int)s_volume; | ||
// compute the new dB level, where 27.71373379 ~ 10^(1/log(2)), and update the | ||
// master gain to it | ||
if ((int)s_volume == 0) { | ||
dBPrevious = 0; | ||
} else { | ||
dBPrevious = powf(27.71373379f, log(((float)s_volume) / 100.0f)); | ||
} | ||
} | ||
param.volume = dBPrevious; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant to the primary change.
{ | ||
bool visiblePlayers[32]; | ||
|
||
if (!client::Client::WallhackActive()) { | ||
GLProfiler::Context p(*profiler, "Non-mirrored Objects"); | ||
RenderObjects(); | ||
RenderObjects(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't refer to Client
directly from the renderer layer. Introduce adequate abstractions in the renderer API.
Thanks for taking the time to reply, yvt. Hoping these problems get fixed 🤞 |
Fixes #807
Original author: dd
I took some time to get Pubovl compatible with the latest
master
, and it has arrived here now.I have tested it ingame, all works fine.
(See the above forum post for more info about Pubovl and its features.)
I thought, well it was made available to the public since a long time ago, with binaries precompiled and available for download from the same forum post above. So what the heck.
There'll always be cheaters in such game(s). Merge this to make admins' lives easier.