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

Fix isPlayerHudComponentVisible incompatible with command showhud #1609

Closed
wants to merge 2 commits into from

Conversation

Unde-R
Copy link
Contributor

@Unde-R Unde-R commented Aug 10, 2020

this should Fix #547 since the command showhud disables the hud and isPlayerHudComponentVisible only checks if the component is visible so It makes no sense.
This should be enough but feel free to say anything.

@patrikjuvonen patrikjuvonen added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Aug 10, 2020
@patrikjuvonen
Copy link
Contributor

patrikjuvonen commented Aug 10, 2020

This is a weird one. There needs to be a distinction between "visible" and "enabled"...

Originally posted by @qaisjp in #547 (comment)

If I understand correctly this pull request would make it impossible to know whether a component has been disabled by script or user. I think this is a negative side effect that must be solved before merge. Also, maybe allow getting showhud state using another function or make new function to know if it's toggled by the user (maybe there is a function already?)

@patrikjuvonen patrikjuvonen requested a review from qaisjp August 10, 2020 20:30
@Unde-R
Copy link
Contributor Author

Unde-R commented Aug 10, 2020

Uhh.. maybe we can work on CCommandFuncs::HUD

Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

I agree we can't merge the current version of this. I've explained why at #547 (comment).

Let's discuss the solution on #547 and we've settled on a solution you can implement it here if you want.

@Unde-R
Copy link
Contributor Author

Unde-R commented Aug 11, 2020

Here is the result:
https://pastebin.com/mAqf4hmU

Comment on lines -1954 to 1958
bool CStaticFunctionDefinitions::IsPlayerHudComponentVisible(eHudComponent component, bool& bOutIsVisible)
bool CStaticFunctionDefinitions::IsPlayerHudComponentVisible(eHudComponent component, bool bOutIsEnabled, bool& bOutIsVisible)
{
bOutIsVisible = g_pGame->GetHud()->IsComponentVisible(component);
bOutIsVisible = g_pGame->GetHud()->IsComponentVisible(component, bOutIsEnabled);
return true;
}
Copy link

Choose a reason for hiding this comment

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

Since you're modifying this function, it's probably better to remove it. We don't put code in CStaticFunctionDefinitions anymore. You can call g_pGame->GetHud()->IsComponentVisible directly. The rest of the PR looks okay.

Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

Had these two comments from a draft review ages ago, I guess I forgot to submit

@@ -1951,9 +1951,9 @@ bool CStaticFunctionDefinitions::ShowPlayerHudComponent(eHudComponent component,
return true;
}

bool CStaticFunctionDefinitions::IsPlayerHudComponentVisible(eHudComponent component, bool& bOutIsVisible)
bool CStaticFunctionDefinitions::IsPlayerHudComponentVisible(eHudComponent component, bool bOutIsEnabled, bool& bOutIsVisible)
Copy link
Contributor

Choose a reason for hiding this comment

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

This, and elsewhere, should be bCheckEnabled.

Comment on lines +279 to +283
if (bOutIsEnabled)
{
if (g_pCore->GetGame()->GetHud()->IsDisabled())
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use if (bIsEnabled && ...->IsDisabled())?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah sure

@patrikjuvonen patrikjuvonen added the feedback Further information is requested label Jan 16, 2021
@patrikjuvonen patrikjuvonen marked this pull request as draft January 16, 2021 22:31
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

This draft pull request is stale because it has been open for at least 90 days with no activity. Please continue on your draft pull request or it will be closed in 30 days automatically.

@github-actions github-actions bot added the stale Inactive for over 90 days, to be closed label Jan 7, 2022
@github-actions github-actions bot closed this Feb 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

This draft pull request was closed because it has been marked stale for 30 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feedback Further information is requested stale Inactive for over 90 days, to be closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isPlayerHudComponentVisible only works with setPlayerHudComponentVisible
3 participants