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

Lilypad: Fix Local Volume control CB function and few other stuffs #781

Merged
merged 3 commits into from Sep 2, 2015

Conversation

Projects
None yet
2 participants
@ssakash
Copy link
Member

commented Aug 20, 2015

There was a wrong value change for vistaVolume in condition for when it is disabled, it sets 100 to the variable. hence when the checkbox gets unchecked, it eventually leads the Local volume control checkbox to be force enabled even when a user disables it at the dialog. the following patch corrects the behavior.

Partial fix for #629

@ssakash ssakash changed the title Lilypad: Fix Local Volume control CB function. Lilypad: Fix Local Volume control CB function and few other stuffs, Aug 21, 2015

@ssakash ssakash changed the title Lilypad: Fix Local Volume control CB function and few other stuffs, Lilypad: Fix Local Volume control CB function and few other stuffs Aug 21, 2015

@turtleli

This comment has been minimized.

Copy link
Member

commented Aug 22, 2015

lilypad commit: Commit is Ok. Fixes the GUI side of the issue. A side note: the local volume control stuff is still broken and it's been broken for at least 5 years. The question is what to do with it. Fix? Remove?

MicroVU commit: Console.Warning? Also, changing only that one might mean that some warnings are red, and some are orange, which would be somewhat inconsistent. I'm not familiar with MicroVU stuff though.

@ssakash

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2015

Lilypad commit: I think it should stay, so anyone could fix it in the future.

MicroVU commit: not really, the other warnings usually affect the game in a certain way causing infinite loop (or) other crash behaviors. This warning is harmless and couldn't really be classified as an warning IMO. It got fixed already, though for some reasons ref said that it should stay. so using a different color just in case so that ref wants to look at such instances of delay slot.

@turtleli

View changes

pcsx2/gui/FrameForGS.cpp Outdated
@@ -402,7 +402,7 @@ static const uint TitleBarUpdateMs = 333;


GSFrame::GSFrame(wxWindow* parent, const wxString& title)
: wxFrame(parent, wxID_ANY, title, g_Conf->GSWindow.WindowPos)
: wxFrame(NULL, wxID_ANY, title, g_Conf->GSWindow.WindowPos)

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 1, 2015

Member

This makes parent an unused parameter. Perhaps this could be done better?

@turtleli

View changes

pcsx2/x86/microVU_Compile.inl Outdated
@@ -207,7 +207,7 @@ __ri void branchWarning(mV) {
incPC(-2);
if (mVUup.eBit && mVUbranch) {
incPC(2);
Console.Error("microVU%d Warning: Branch in E-bit delay slot! [%04x]", mVU.index, xPC);
Console.WriteLn(Color_Orange, "microVU%d Warning: Branch in E-bit delay slot! [%04x]", mVU.index, xPC);

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 1, 2015

Member

If the user needs to know about it but it's not of major concern then I think Console.Warning should be used.
If the user doesn't need to know about it then I think DevCon.Warning should be used.

ssakash added some commits Aug 21, 2015

MicroVU: use DevCon.Warning for E-bit delay slot.
The following E-bit delay slot warning initially used a console error message which has a sort of bright red which would cause users to provide a lot of attention towards to it, whereas it isn't much to be bothered since it doesn't cause any issues on the game according to refraction it was already dealt with on the code.

So, use a proper warning indicator instead of a error log message.
don't minimize GS window along with main window.
previously the GS window (rendering window) also gets minimized with the Main window (GUI) whenever it gets minimized. many users didn't like this behavior, hence make it so that GS window doesn't get minimized along with the main one.

The parent window pointer parameter is no longer needed, since the parent parameter at WX Frame has been replaced with Null which doesn't force minimize it due to no parent relationship.

@ssakash ssakash force-pushed the ssakash:patch-49 branch 2 times, most recently Sep 2, 2015

@ssakash

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2015

MicroVU: decided to go with DevCon.Warning, that seems more suitable for the scenario.

GSFrame: That parameter is pretty much pointless after the relationship to NULL, removed it. do you agree ?
ssakash@84ab1fb

@turtleli

This comment has been minimized.

Copy link
Member

commented Sep 2, 2015

Yes, that's what I was looking for. Perhaps squash the 2 GSFrame commits? I'll retest this later.

@ssakash ssakash force-pushed the ssakash:patch-49 branch to b5acece Sep 2, 2015

@ssakash

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2015

sure.

turtleli added a commit that referenced this pull request Sep 2, 2015

Merge pull request #781 from ssakash/patch-49
Fix Windows Lilypad local volume control GUI checkbox.
Make MicroVU E-bit delay slot warning a Dev warning.
Don't minimise GS Window if main window is minimised.

@turtleli turtleli merged commit 288931d into PCSX2:master Sep 2, 2015

@turtleli

This comment has been minimized.

Copy link
Member

commented Sep 2, 2015

Thanks.

I edited your initial comment so the Lilypad issue doesn't autoclose - your commit only fixes the GUI side of things. Currently it's better to have local volume control off - Windows remembers the volume settings. Having local volume control on interferes with Windows.

Side note: consider naming your branches meaningfully. It makes it easier for others to know/remember what the branch is for.

@ssakash ssakash deleted the ssakash:patch-49 branch Sep 3, 2015

@ssakash ssakash restored the ssakash:patch-49 branch Sep 6, 2015

@ssakash ssakash deleted the ssakash:patch-49 branch Sep 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.