Project

General

Profile

Actions

Emulator Issues #5689

closed

Merge request.

Added by john.s.peterson over 11 years ago.

Status:
Fixed
Priority:
Normal
Assignee:
% Done:

0%

Operating system:
N/A
Issue type:
Bug
Milestone:
Regression:
No
Relates to usability:
No
Relates to performance:
No
Easy:
No
Relates to maintainability:
No
Regression start:
Fixed in:

Description

Actions #1

Updated by delroth over 11 years ago

"Adding batch build files."

Why not use msbuild instead of calling devenv? Also, please put all the scripts in a subdirectory to avoid having a lot of files in Source/.

"Changing reported iso file size for sparse and compressed files"

I'm not sure if st_blocks unit size == 512 is a valid assumption to make, especially with 4K sectors disks. Is there any reference to the fact that st_blocks always counts 512 bytes blocks?

Actions #2

Updated by NeoBrainX over 11 years ago

  • Random "// TODO: fix this" comments in 0680e0052ec5?
  • Why would we want OSD messages to be in the logfile? If any info is missing in a log file (hint: i see no reason why it would), make the corresponding code log it instead of using OSD messages.
  • I don't agree that Release builds need a "restart in debug mode" option. I don't see any need for this option at all, but if anything it shouldn't be in non-developer builds.
  • afdfeaeebe34, again, has lots of different changes in one file... what's so hard to keep stuff in separate commits, especially when working with Git?
  • possibly other stuff, but too lazy to check which change in 0680e0052ec5 affects which files.

Also, given your past contributions to Dolphin I'm kinda opposed against merging any of your changes back, but that's just my personal opinion.

Actions #3

Updated by rdragoon over 11 years ago

I really like what i see on 970514485071.

In 0680e0052ec5 why did you add "if (!IsUsingWiimote(0))" to movie.cpp at line 1015? It seems unnecessary. It won't actually change anything, since as far as i'm aware, there are no multi disc wii games, and as of now, recording gc controller and wiimote at the same time doesn't work anyway (or does it, and it was just that check which you disabled that was causing it to stop?). Still, it doesn't make sense to break support for changing discs while playing back gc+wiimote at the same time, even if it'll never be used. Also, even if we did want to do that, why not just use "if (!tmpHeader.bWii)"?

Other than that, i can't wait to try wiimote recording.

Actions #4

Updated by NeoBrainX over 11 years ago

Okay. I won't go into explaining obvious stuff to you again. Either you're not understanding stuff on purpose or you should think about my comments some more, because it's really not that hard to grasp what I'm talking about.

Actions #5

Updated by rdragoon over 11 years ago

I agree, but even without disc change support it's an improvement.

Sure, but is there some reason we need to disable it? It should just work as is for wii games, so long as it uses a gc controller (since i never implemented it for wiimote recording).

Actions #6

Updated by Sonicadvance1 over 11 years ago

given your past contributions to Dolphin I'm kinda opposed against merging ... your changes
What contributions?

The problems specifically in this issue on your last merge request.
http://code.google.com/p/dolphin-emu/issues/detail?id=5041

Actions #7

Updated by Sonicadvance1 over 11 years ago

"I think all your crazy patches need to be split up in to separate patch files since a lot of the changes affect many different things."
-- That one.

"Seconding what Sonicadvance1 said... You've changed all kinds of unrelated things, what about first integrating m+ and then talk about the other stuff?
Also, your committing style is a bit worrying.. d7d99b4a7111 for example changes all kinds of unrelated stuff, if anyone were to find a regression in there it'd be awkward to find out what's wrong if one's not familiar with the code."
-- That one.

"For what it's worth, I agree. The motion plus functionality is what's important. Other preference changes is only one person's opinion at this point. Trying to get the best possible motion plus feature alone is what should be the focus."
-- That one.

"JPeterson: From the looks of things, nothing is ever going to get done here until you separate the emulated motion plus code form the rest of what you've done there. If you do not wish to do that, that's fine, but I wouldn't expect anything to ever be added to master otherwise."
-- That one.

"Please separate the changes marked with * out into their own branch:

Improved Wiimote emulation.
Emulated Wiimote cleanup.
Re-add wiimote packets sniffer.
RawInput mouse backend.
*General cleanup and additions.
*Non-modal input dialogs.
*Resizable and by default larger cheat options dialog. Gekko codes moved to first tab.
*Graphics hotkeys cleanup.
Support for IR calibration from isoini database.
*Additional isoini save options
*Add option to load savestate by date saved, and overwrite oldest savestate.
*Savestate cleanup.
*Benchmarking from .dtm.
*Demo loading cleanup
*Hotkey cleanup.
*Resources cleanup.
*Build cleanup.

We can merge the ones marked with * at a later stage."
-- That one.

"JP: The point of splitting is to make it easier reviewable, so its not better to wait, because thats what we did for the past month since you opened this issue.
The faster you split them up the faster people can (and will) review it - and the faster it will also make its way into master."
-- That one.

"Still has too much unnecessary stuff in it.
Just looking over the first changes: Randomly commenting out code, adding unused parameters."
-- That one.

Too much unnecessary crap all merged in to one branch. One pull request per feature. Don't ball it all up in to a single patch that has everything in it. It makes it a pain in the ass to review especially considering if we don't want /everything/ you've done.

There are tons of things that I could continue on about, but I can't take my entire day doing it.

Actions #8

Updated by tommyhl2.SS over 11 years ago

John: There are at least 5 solid developers that have given their opinions already between this thread and the original and they've all told you the same thing. I don't know what other opinions you're looking for...

Actions #9

Updated by NeoBrainX over 11 years ago

  • Status changed from New to Won't fix

Okay this is just dumb. You're sitting around on IRC and if you really weren't understanding stuff you'd just ask there so I'm assuming you're just kidding us.

Anyone feel free to disagree with me on this and re-open this issue, but IMO this deserves no further attention.

Actions #10

Updated by rdragoon over 11 years ago

I'll handle this. There are some nice changes in here I'd like to keep. No sense trashing them all because the author is being a bit silly, when it's not that hard to deal with.

Actions #11

Updated by Autoran1 over 11 years ago

Remembering all his previous commits, just can say Please DON't merege this...

Actions #12

Updated by rdragoon over 11 years ago

You don't want useful stuff to be merged because you don't like the author? Doesn't that seem just a bit petty?

Actions #13

Updated by Autoran1 over 11 years ago

Think whatever you want about this changes, but i saw when a hundred commits first is some huge thing and each next is trying to fix previous one and makes things even worse than before, what can i say? Yes i don't like this author.

Actions #14

Updated by rdragoon over 11 years ago

I see 8 commits, none of which fix anything broken in a previous commit, but ok.

Actions #15

Updated by death2droid over 11 years ago

If this is the same guy i think itt is, back in the SVN days of Dolphin Emu he use to make hundreds of commits in one go that created more problems then fixed, and like Autoran1 said, it ended up being lots of commits fixing his previous commits, hence why he actually ended up being removed as a committer. If your going to commit to the master branch these make sure you do go over them with a fine comb and make sure they do neccesary things and not just bulking up the code for no real reason.

Actions #16

Updated by death2droid over 11 years ago

and on another note, looking at his changes, he seems to introduce alot of windows only code / ends up ifdef'ing a lot of the code which for a mutli system emulator really isn't the best thing

Actions #17

Updated by death2droid over 11 years ago

Looking at it further, it shouldnt be disregarded completely.
http://code.google.com/r/jpeterson57-dolphin/source/detail?r=1909c33939855e9b696c891f0d3d9f596f2abfa0&name=master <-- the wiimote changes in here are definitely worth keeping, sooo Wiimote.cpp , EmuSubroutines.cpp , WII_IPC_HLE_Device_usb.cpp

Actions #18

Updated by rdragoon over 11 years ago

Yeah, those are the changes i care about.

Actions #19

Updated by rdragoon over 11 years ago

Save oldest state doesn't work. If 8 slots are already being used, it just saves to slot -1.

Actions #20

Updated by rdragoon over 11 years ago

I don't have a github account, and i'm not making one just to discuss this, sorry.

It mostly works now, but still fails miserably when using it rapidly (like, faster than once per second). I've also seen instances of saving to slot -1 when holding down the hotkey.

Actions #21

Updated by rdragoon over 11 years ago

Turns out i'm just too lazy to go through all of this mess. I merged a couple of your commits, and i'd still like to merge the hotkey one, if you fix it, but otherwise i just don't care to go through the rest. If you want to clean up the commits to be more manageable, maybe i'll change my mind.

Actions #22

Updated by parlane over 11 years ago

  • Status changed from Won't fix to Fixed
Actions #23

Updated by john.s.peterson almost 11 years ago

The merge request branch is moved from Google Code to https://github.com/mirror/dolphin-emu/pulls

Actions

Also available in: Atom PDF