Alcaro commented 7 years ago

+ const char systemdirtmp; + bool worked = environ_cb(RETRO_ENVIRONMENT_GET_SYSTEM_DIRECTORY,(void )systemdirtmp);

You forgot an &

+ if(!worked)memset(systempath,0,4096);

Waste of time, just wipe the first byte

+#include <sys/stat.h>

Not available on Windows

+bool using_bootloader = false;

Is that present in savestates? If yes, do we want to break savestates for such a trivial thing? If no, does that break anything?

+ fp = fopen(path.c_str(), "r");

"rb"

void GB::reset() { void GB::Priv::on_load_succeeded(unsigned flags)

Code duplication, better split to a separate function. Additionally, lack of off-by-default core option for this thing; I don't want a mandatory two second jingle on every game load, that's a waste of time.

Finally, is this thing upstreamed? I'm not really comfortable including behavioral changes that aren't reviewed by upstream.

Alcaro commented 7 years ago

Oh, and this looks like a violation of core/front separation. Public global variables outside headers, improper namespacing, file IO outside libgambatte/libretro/, that's the way to spaghetticode.

And I suspect this breaks the GBA mode option. Someone better check that.

meepingsnesroms commented 7 years ago

Fixed &.

Fixed stat/windows.

Fixed open as binary.

Fixed array clear.

If there is no bootloader in your system directory it will run the same as before.(no jingle)

Does not break gba mode if bootloader is not present,but can add an option that disables bootloaders when using gba mode.

As for code duplication,that was there before I did anything,I just left it the way it was.

How else am I supposed to do file IO?

Dont know about savestates but if no bootloader is present they will work just as before.

inactive123 commented 7 years ago

@Alcaro Regarding upstream - it's pretty much dead in activity and has been so for years. Not worth it to hold back new code for that reason if it isn't even maintained regularly.

sergiobenrocha2 commented 7 years ago

On linux:

g++ -c -o libgambatte/src/bootloader.o libgambatte/src/bootloader.cpp -O3 -fno-exceptions -fno-rtti -DHAVE_STDINT_H -std=c++98 -fPIC -D__LIBRETRO__  -DHAVE_STDINT_H -DHAVE_INTTYPES_H -DINLINE=inline -DVIDEO_RGB565 -DHAVE_NETWORK -Ilibgambatte/src -Ilibgambatte/src/../include -Ilibgambatte/src/../../common -Ilibgambatte/src/../../common/resample -Ilibgambatte/src/../libretro
libgambatte/src/bootloader.cpp: In function ‘bool exist(const string&)’:
libgambatte/src/bootloader.cpp:10:42: error: expected ‘)’ before ‘{’ token
     if ((file = fopen(name.c_str(), "r")){
                                          ^
libgambatte/src/bootloader.cpp: In function ‘bool loadbootloader(bool)’:
libgambatte/src/bootloader.cpp:68:51: error: ‘memcpy’ was not declared in this scope
    memcpy(rombackup,(uint8_t*)addrspace_start,size);
                                                   ^
libgambatte/src/bootloader.cpp: In function ‘void call_FF50()’:
libgambatte/src/bootloader.cpp:93:64: error: ‘memcpy’ was not declared in this scope
       memcpy((uint8_t*)addrspace_start,rombackup,bootloadersize);
                                                                ^
make: *** [libgambatte/src/bootloader.o] Error 1

meepingsnesroms commented 7 years ago

I am fixing it right now.

meepingsnesroms commented 7 years ago

The savestate problem is fixed and everything compiles now. (saving in bootloader and loading in game or saving in game and loading in bootloader)

sergiobenrocha2 commented 7 years ago

g++ -c -o libgambatte/src/bootloader.o libgambatte/src/bootloader.cpp -O3 -fno-exceptions -fno-rtti -DHAVE_STDINT_H -std=c++98 -fPIC -D__LIBRETRO__  -DHAVE_STDINT_H -DHAVE_INTTYPES_H -DINLINE=inline -DVIDEO_RGB565 -DHAVE_NETWORK -Ilibgambatte/src -Ilibgambatte/src/../include -Ilibgambatte/src/../../common -Ilibgambatte/src/../../common/resample -Ilibgambatte/src/../libretro
libgambatte/src/bootloader.cpp: In function ‘bool loadbootloader(bool)’:
libgambatte/src/bootloader.cpp:74:51: error: ‘memcpy’ was not declared in this scope
    memcpy(rombackup,(uint8_t*)addrspace_start,size);
                                                   ^
libgambatte/src/bootloader.cpp: In function ‘void call_FF50()’:
libgambatte/src/bootloader.cpp:114:64: error: ‘memcpy’ was not declared in this scope
       memcpy((uint8_t*)addrspace_start,rombackup,bootloadersize);
                                                                ^
libgambatte/src/bootloader.cpp: In function ‘void uncall_FF50()’:
libgambatte/src/bootloader.cpp:121:68: error: ‘memcpy’ was not declared in this scope
    memcpy((uint8_t*)addrspace_start,bootromswapspace,bootloadersize);
                                                                    ^
make: *** [libgambatte/src/bootloader.o] Error 1

meepingsnesroms commented 7 years ago

Sorry forgot string.h my compiler was more forgiving so it worked for me.

sergiobenrocha2 commented 7 years ago

g++ -c -o libgambatte/src/gambatte.o libgambatte/src/gambatte.cpp -O3 -fno-exceptions -fno-rtti -DHAVE_STDINT_H -std=c++98 -fPIC -D__LIBRETRO__  -DHAVE_STDINT_H -DHAVE_INTTYPES_H -DINLINE=inline -DVIDEO_RGB565 -DHAVE_NETWORK -Ilibgambatte/src -Ilibgambatte/src/../include -Ilibgambatte/src/../../common -Ilibgambatte/src/../../common/resample -Ilibgambatte/src/../libretro
libgambatte/src/gambatte.cpp: In member function ‘void gambatte::GB::reset()’:
libgambatte/src/gambatte.cpp:59:57: error: ‘printf’ was not declared in this scope
    printf("Bootloader exists:%d\n",(int)bootloaderexists);
                                                         ^
libgambatte/src/gambatte.cpp:70:7: error: ‘memset’ is not a member of ‘std’
       std::memset((void*)(state.mem.ioamhram.get() + 0x100),0x00,0x100);
       ^
libgambatte/src/gambatte.cpp: In member function ‘void gambatte::GB::Priv::on_load_succeeded(unsigned int)’:
libgambatte/src/gambatte.cpp:93:57: error: ‘printf’ was not declared in this scope
    printf("Bootloader exists:%d\n",(int)bootloaderexists);
                                                         ^
libgambatte/src/gambatte.cpp:104:7: error: ‘memset’ is not a member of ‘std’
       std::memset((void*)(state.mem.ioamhram.get() + 0x100),0x00,0x100);
       ^
make: *** [libgambatte/src/gambatte.o] Error 1

meepingsnesroms commented 7 years ago

Why is you compiler giving errors on those? Most compilers let you use stdlib functions even if the header is not included.

sergiobenrocha2 commented 7 years ago

Ok, it's working now. Very nice!!!

Not really a demand (since there's no standard in other cores), it should be nice to load either gb_bios.bin / gbc_bios.bin or them with no-intro names:

[BIOS] Nintendo Game Boy Boot ROM (World).gb [BIOS] Nintendo Game Boy Color Boot ROM (World).gbc

meepingsnesroms commented 7 years ago

Someone else can do that if they want but those names are inaccurate and I wont support them. (.gb/.gbc is misleading since there not runable roms,also its not a bios its a bootloader,the brackets also may confuse some programs)(They work anyway you just have to rename them once.)(Also matches the "gba_bios.bin" name format that everyone that uses gameboy emus already knows.)

meepingsnesroms commented 7 years ago

Can this be merged now?(when I do something I need to finish it or I get very stressed,I have been stressing about this for an hour now.)

andres-asm commented 7 years ago

I think people who can review this might be in bed atm

On Thu, Feb 2, 2017, 10:57 PM meepingsnesroms notifications@github.com wrote:

Can this be merged now?(when I do something I need to finish it or I get very stressed,I have been stressing about this for an hour now.)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/libretro/gambatte-libretro/pull/67#issuecomment-277158873, or mute the thread https://github.com/notifications/unsubscribe-auth/ABpC0Jwvx90JneMMlD9nyQpmRN7dTRsXks5rYqW6gaJpZM4L1w0J .

inactive123 commented 7 years ago

Give Alcaro a few hours to get out of bed so he can take another look at this, he is from Sweden so his timezone is GMT+1.

Alcaro commented 7 years ago

Does not break gba mode if bootloader is not present,but can add an option that disables bootloaders when using gba mode.

Pretty sure a real GBA uses a third BIOS. Best solution would be to follow that pattern.

As for code duplication,that was there before I did anything,I just left it the way it was.

Not in those functions, nearly every line is different in the original versions of those (and the remainder are variable declarations that can't be in a function). That bootloader reset thingy, on the other hand, is identical.

How else am I supposed to do file IO?

Load file in the frontend code, create some gambatte::setBios() function to get the BIOS where it belongs. Easiest way would be having that function take three pointers, one each for GB/GBC/GBA. NULL is allowed and means 'if this BIOS would be correct, ignore it and use HLE'.

Yes, that's more effort. No, I don't care, doing things right is always more short-term effort.

Dont know about savestates but if no bootloader is present they will work just as before.

"I don't know" is not the correct answer to this kind of questions.

Regarding upstream - it's pretty much dead in activity and has been so for years.

Fair enough.

Most compilers let you use stdlib functions even if the header is not included.

We can't depend on nonstandard compiler properties like that. And you're mixing up stdlib.h and cstdlib in a strange way, too; former puts things in the global namespace, latter puts them in std::.

+#include "string.h"

"" is for project-internal headers. For system headers, better use <>.

Can this be merged now?(when I do something I need to finish it or I get very stressed,I have been stressing about this for an hour now.)

That's not how development works. You will allow the code review to take the time it takes.

Alcaro commented 7 years ago

You will allow the code review to take the time it takes.

This applies doubly if you've asked a question, for example "How else am I supposed to do file IO?". Expecting a merge without awaiting answers for your own questions makes no sense whatsoever.

meepingsnesroms commented 7 years ago

What is wrong with the brackets,you did this too. In libretro.cpp #include <gambatte.h> Project header as system header.

sergiobenrocha2 commented 7 years ago

Pretty sure a real GBA uses a third BIOS. Best solution would be to follow that pattern.

indeed, gba displays gbc boot for both gb and gbc games

meepingsnesroms commented 7 years ago

Looks like gba mode just sets Z80 register B to 1 so the normal gbc bios can be used with it.

Alcaro commented 7 years ago

What is wrong with the brackets,you did this too.

That's the opposite direction, it's a lot more commonly accepted. It's also the project including its own public header.

But yes, header includes in C/C++ are a bit of a mess.

set_bootrom_directory((char*) systemdirtmp);

No const removal unless interacting with a legacy API or a userdata pointer. Actually, better use std::string all the way.

And that's still file I/O in the core.

if(path[path.length() - 1] != '/')path += '/';

I'm pretty sure that breaks on Windows.

Looks like gba mode just sets Z80 register B to 1 so the normal gbc bios can be used with it.

It's either different BIOS or different initial hardware state. Former sounds more likely to me, but if you've verified it's the latter, then it is.

Another complaint: Nearly all of Gambatte is in a class, and there can be multiple instances of that class, so I was able to create this in about five minutes: gambatte-dual I like it that way. Those globals should be member variables.

Alcaro commented 7 years ago

patching a bunch of opcodes across the bios is a quite strange way to use the normal one.

meepingsnesroms commented 7 years ago

What do you think the gba version of the gbc bios is?(Probably what I just did.)

meepingsnesroms commented 7 years ago

If you want you can put them in a class,that is just nitpicking. It works,its readable and has a well defined interface,unless it has an actual issue I am done working on it.(Issue being crashing,ingame differences,netplay broken)

meepingsnesroms commented 7 years ago

Windows can handle forward and backward slashes. MGBA vfs.h `#ifdef _WIN32

include

define PATH_SEP "/" // Windows can handle slashes, and backslashes confuse some libraries

else

define PATH_SEP "/"

endif`

Alcaro commented 7 years ago

What do you think the gba version of the gbc bios is?(Probably what I just did.)

Quite possible. Or perhaps it shifts everything around. Or maybe it does something completely different. The only way to find out is to find the real GBA-GBC BIOS.

It feels fairly ugly to patch BIOSes like that, but if the GBA-GBC BIOS isn't dumped or commonly circulated, this is good enough. Still, it'd be better to look for gbagbc_bios.bin first and use this patching as a fallback.

If you want you can put them in a class,that is just nitpicking.

Global variables are well known in the field of programming to reduce readability and maintainability. Especially when the existing core is object oriented; doing nearly everything on the Gambatte object, except this one thing, will confuse everyone.

The ability to instantiate multiple Gambattes can be turned into splitscreen multiplayer, like TGB Dual. Do you want to close that possibility?

Yes, there are some globals already; libretro itself isn't object oriented, we have no choice. Such globals are unfortunate and should be confined the smallest region possible.

Windows can handle forward and backward slashes.

path+="/" is fine, path.endswith("/") may break. Perhaps it does work, but most cores check for both, so I'm skeptical. Or just remove the if, I've never seen double slashes cause trouble.

Finally, still no core option toggle, and still doing file IO outside libretro.cpp. If the main ROM, SRAM, savestates and everything else can be in memory, why can't the BIOS?

meepingsnesroms commented 7 years ago

I can fix the class issue. Bios has to be loaded after init to know whether to use gb or gbc bootloader,I can make a function pointer call that returns an array like this. void (*get_raw_bootloader_data)(bool/*gbcver*/,uint8_t* /*data*/); Then the frontend just supplies its own get_raw_bootloader_data function. RetroArch already adds the slash at the end of the path,this is just extra safety incase that changes,do you want it to be removed?

Alcaro commented 7 years ago

Bios has to be loaded after init to know whether to use gb or gbc bootloader void (get_raw_bootloader_data)(bool/gbcver/,uint8_t /data/);

Or load both GB and GBC BIOSes and let core pick which one to use.

But this is a good choice as well, doesn't matter to me which one you choose. Actually, this one is better, it doesn't touch the unneeded file.

Nitpicks:

It's better to use some tri-value flag, to ask for the GBA-GBC BIOS. HLE requires knowing if it should emulate GBC or GBA registers, so it might as well pass that on. Strange how there's no GB/GBC/GBA enum in gambatte.h.
Someone has to keep track of the expected filesize. Probably better to add that as a third argument.
Make it return a boolean telling whether it found the requested file; if false, use HLE. The core opt check could fit in there as well. Or maybe pass NULL as the callback, I have no strong preference here.
No need to comment out the argument names.

I also recommend disabling savestates completely until the BIOS ends, using RETRO_SERIALIZATION_QUIRK_MUST_INITIALIZE. There's no need for savestates during a two second boot sequence, and it's the easiest and safest way to ensure we don't get any silly bugs and unexpected savestate format incompatibilities.

RetroArch already adds the slash at the end of the path,this is just extra safety incase that changes,do you want it to be removed?

libretro should specify whether trailing slash is banned, required or optional, so we know what to do. Lazy spec.

Check, always append, never appending, I'm fine with anything. Just don't treat / and \ differently on Windows.

meepingsnesroms commented 7 years ago

There are no gba or gbc registers gba just puts a 1 in z80 register b.(the hardware is exactly the same,no special features)

meepingsnesroms commented 7 years ago

I already made special functions to allow save/load state in the bios to work properly.(No need to worry about breaking old save states,I used the gbc hardware register that did this to store the value)

The get file function does return a bool in the final version.

Alcaro commented 7 years ago

Make it return a boolean telling whether it found the requested file; if false, use HLE.

Hey neat, you did that already

+ if(isgbc){ + path += "gbc_bios.bin"; + size = 0x900;

Which idiot actually inserted that 0x100 byte hole in the 0x800 byte BIOS, and didn't even put in the mandatory Nintendo logo

sigh, let's just keep using the silly bios, no point making up our own standards. even if they're better.

gba just puts a 1 in z80 register b

Yes, which we agreed is implemented via varying BIOS, hence there's three different relevant BIOSes.

Fully object oriented

Clear improvement, but

+extern bool usebootloaders; +extern bool (get_raw_bootloader_data)(bool/gbcver/,uint8_t /data/);

not quite finished. Add a function to gambatte.h taking that function pointer; if NULL, use HLE. I see no real use for usebootloaders, we already have several ways to check if LLE should be available; the fewer, the better, easier to test them all. (For the same reason, setting the callback to NULL should be replaced with a function that always returns false.)

100% object oriented would also mean a void* userdata on that callback, which is passed along with that fnptr and into it, and otherwise unused by the core. We don't really need it for libretro, but it's good practice to add userdata to all callbacks.

No need to worry about breaking old save states,I used the gbc hardware register that did this to store the value

If you're confident it works, including saving states with LLE enabled and loading with it disabled (other than states made during a now-disabled BIOS, but we don't need to care about THAT), then sure, why not.

Core options are RETRO_GET_VARIABLE/etc. I have no idea why they have two names.

meepingsnesroms commented 7 years ago

I haven't tested using lle to hle but the way I check if the bootloader is inactive (if reg 0xFF50 != 0) is always true on hle so it is just like finished the bootloader.

Usebootlader enables bootloaders the get function is required if usebootlader is true.

What about void userdata the array to dump the bootloader into is already sent to it since that is all it needs to write or ever will need to write.

Usebootlader will be set with a core option so it is needed.(just setting the get function to null seems very hacky, although it already detects and uses hle if it is null)

The global variables apply to all emulated Gameboys so they need to stay global.

meepingsnesroms commented 7 years ago

Ok,it compiles,supports multiple instances and can be turned off and is not mixed with the frontend.

Alcaro commented 7 years ago

that is all it needs to write or ever will need to write

For our usecase, yes. But perhaps we - or someone else who finds our repo - want to do something different in the future, something that requires a userdata.

The global variables apply to all emulated Gameboys

They shouldn't.

meepingsnesroms commented 7 years ago

These are hypothetical future scenarios,there are an infinite number of those we can't accommodate them all.

There is no point to using a bootloader on gb1 but not on gb2,so the globals are fine with me.

meepingsnesroms commented 7 years ago

Are those 2 tiny things going to prevent it from being merged? I would not have done the class fix,but you convinced me with the idea of multiple instances running at once,these variables don't even hurt compatibility it is just for looks.(most people don't even see the code)

Alcaro commented 7 years ago

So you're refusing to follow the coding style, generally accepted principles of software engineering, and direct orders from the maintainers?

inactive123 commented 7 years ago

@meepingsnesroms I think Alcaro's entire reasoning here is that the core was clearly designed from the start to be class-based and adherent to specific C++ guidelines, adding a bunch of global variables everywhere would be regressive for this specific codebase.

Please follow his code suggestions. This should not be a hard thing for you to do.

Alcaro commented 7 years ago

I'm not interested in the short-term functional state of this core, I care only about long-term results. While merging this PR would be a short-term improvement, reducing maintainability will scare off more contributors long-term. I cannot accept that.

meepingsnesroms commented 7 years ago

This is the last style based change,merge it or don't!!!

Alcaro commented 7 years ago

Is that a threat

meepingsnesroms commented 7 years ago

Of what,quitting? Then yes,else no, I am just done wasting my time on that.

meepingsnesroms commented 7 years ago

So should I just delete this?

inactive123 commented 7 years ago

@meepingsnesroms @Alcaro will review it tomorrow. Let's not get overdramatic here and let yourself be guided by emotions, and that goes for both parties here. I'm sure that we can come to some kind of mutual understanding tomorrow and that most of the serious issues associated to this PR have kind of subsided at this point.

Alcaro commented 7 years ago

The most important thing if you want to contribute to someone else's project isn't being right the first time. It's not being a good programmer.

It is being willing to learn, and to accept feedback.

We don't mind flawed PRs, they can be fixed. But only if you want to. If you're not willing to follow our advice, we're gonna have a problem.

That said, I can find the following issues:

Callback should take userdata. No, I don't care about 'libretro.cpp doesn't need that', it's called future-proofing.
Callback should take a length argument.
Callback should take tri-state value for which bootrom to return. Patching goes in the core; if the callback doesn't find a GBA-GBC ROM, core should request and patch the GBC bootrom.
Like most C++ projects, Gambatte coding style declares variables on first use, not all at top.
It is unclear why setBootloaderEnabled exists when the callback can return false (or, equivalently, be NULL). Multiple ways to do the same thing is generally not a good thing. Easiest fix should be reading the core opt in the callback, but there are several equally good methods.
There's no reason to comment out or otherwise remove argument names from function pointers.
Bootloader should be in namespace gambatte::.
p->cpu.mem.bootloader.whatever is ugly, especially when there's five of it; deep pointer mazes suggest bad separation of responsibilities.
Same goes for GB::Priv::on_load_succeeded. Code duplication and pointer maze.

I can't think of a good fix for the last two, that saveState thingy is already confusing. I'll clean them up at some point, and I'll accept those functions being merged in their current state; I'll fix them up later.

The others should be straightforward. I don't care if they're fixed before or after merging this, as long as someone does it.

meepingsnesroms commented 7 years ago

I am fine with that,I was mad because I felt that this feature was being held as a hostage to force those changes.

I will do what you listed other than the GBA GBC BIOS.(since it has not been dumped yet and probably never will be)

I need more explanation on what the userdata would be for in a future use.(since all that function does is dump a file into a buffer)

I will fix them,but this needs to be merged.

Aerocatia commented 7 years ago

Never say never. The SGB2 bootrom was dumped 6 years after the first SGB bootrom even though it could be done with a similar method. The only thing stopping the GBA-GBC rom being dumped as far as I'm aware is needing someone willing to put the hardware together and sacrifice a GBA for something rather unimportant.

meepingsnesroms commented 7 years ago

I don't understand why it matters,since I made a gba gbc bios and as long as it is 100% compatible,looks the same and works the same,it is the same,who cares if the bytes are different.

If it ever does happen and I still work on retroarch I will still add it though.

meepingsnesroms commented 7 years ago

I must squish the cuddly cuttly cuttlefish because I wish to give a squish to that fish.

libretro / gambatte-libretro

Support using official bootloader with startup logo #67

include

include

define PATH_SEP "/" // Windows can handle slashes, and backslashes confuse some libraries

else

define PATH_SEP "/"

endif`