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

My own version of Gnuboy. New functionalities added #2

Closed
wants to merge 1 commit into from
Closed

My own version of Gnuboy. New functionalities added #2

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 23, 2017

-Screenshots
-Screencasting
-Console/debugger (beta)

A console will appear on the screen when Gnuboy is running. Type 'help' and you will see the available commands and how to take both screenshots and do screencasts.

Important: enable debugging in order to use a few of the functionalities within the debugger. Otherwise, they won't work (Error messages will appear, while normal execution will go on without any results).

-Screenshots
-Screencasting
-Console/debugger (beta)
@rofl0r
Copy link
Owner

rofl0r commented May 23, 2017

cool!
i'm generally willing to add your enhancements, but merging a single 1000 lines patch is against all my principles. therefore i'd like to ask you to go over your changes and make one commit per new feature or logical change, so we can bring each commit to a size that's reasonable to review.
does that sound acceptable?

Copy link
Owner

@rofl0r rofl0r left a comment

Choose a reason for hiding this comment

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

i added some comments. i'll do further review once you've come up with clean one-feature-per-commit commits.

@@ -7,7 +7,7 @@ const static byte cycles_table[256] =
1, 3, 2, 2, 1, 1, 2, 1, 5, 2, 2, 2, 1, 1, 2, 1,
1, 3, 2, 2, 1, 1, 2, 1, 3, 2, 2, 2, 1, 1, 2, 1,
3, 3, 2, 2, 1, 1, 2, 1, 3, 2, 2, 2, 1, 1, 2, 1,
3, 3, 2, 2, 3, 3, 3, 1, 3, 2, 2, 2, 1, 1, 2, 1,
3, 3, 2, 2, 1, 3, 3, 3, 3, 2, 2, 2, 1, 1, 2, 1,
Copy link
Owner

Choose a reason for hiding this comment

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

why was this changed? please put into a single commit explaining.

@@ -26,7 +26,7 @@ FB_LIBS =
SVGA_OBJS = sys/svga/svgalib.o sys/pc/keymap.o @JOY@ @SOUND@
Copy link
Owner

Choose a reason for hiding this comment

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

please no gratuitous modechanges, thanks.
(refering to Makefile.in 100644 → 100755, and the same for almost all other files)

@@ -45,20 +45,19 @@ all of the following and much more:
Wave pattern memory corruption when sound channel 3 is played.
Pad, timer, divide counter, and other basic hardware registers.
CGB double-speed CPU mode.
Sorting sprites by X coordinate in DMG mode.
Copy link
Owner

Choose a reason for hiding this comment

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

why was this block moved to section below?

@@ -0,0 +1,22 @@
# mygnuboy
My own version of gnuboy emulator
Copy link
Owner

Choose a reason for hiding this comment

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

i dont think we want to change the name to "mygnuboy". it should stay as close to laguna's original as possible.

scrot
ffmpeg

Both of them available via apt-get
Copy link
Owner

Choose a reason for hiding this comment

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

not all the world is debian/ubuntu.



scrot
ffmpeg
Copy link
Owner

Choose a reason for hiding this comment

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

if you want to add third-party dependency, then please make them optional. ffmpeg is a huge download and takes considerable time to build.

@@ -641,7 +643,7 @@ int cpu_emulate(int cycles)
break;
case 0x3C: /* INC A */
INC(A); break;

Copy link
Owner

Choose a reason for hiding this comment

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

please no gratuitous whitespace changes.

void sound_advance(int cnt);
void cpu_timers(int cnt);
int cpu_emulate(int cycles);
int return_globalpc();
Copy link
Owner

Choose a reason for hiding this comment

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

what's the point of removing these decls ?
it looks more and more as if you just copied the "community edition" gnuboy over this one.

* Acknowledging previous NMUs, thanks Moritz! closes: #711112.

-- Davide Puricelli (evo) <[email protected]> Sun, 30 Jun 2013 16:54:28 +0200

Copy link
Owner

Choose a reason for hiding this comment

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

i dont think debian specific files have any reason to go into this repo.

@rofl0r rofl0r mentioned this pull request May 24, 2017
@rofl0r
Copy link
Owner

rofl0r commented May 24, 2017

since you deleted your fork of my repo, i assume you're not interested of cleaning up your work. thanks for nothing.

@rofl0r rofl0r closed this May 24, 2017
@ghost
Copy link
Author

ghost commented May 24, 2017

Hello rofl0r!

Of course I am interested on it, but since this is the first time I ever participate in a free software project and I realised I did a few things wrong, I will work on what I wanted to commit so that it fits and respects all the things you remarked before.

I still want to contribute with a debugger functionality. Regarding both the screencast and the snapshot functionalities, which could be an appropriate approach when trying to keep the program available for several distributions?

I will try to commit every change so that everyone can keep track of what I have done in a proper way.

Thank you, and sorry for the inconvenience :)

@rofl0r
Copy link
Owner

rofl0r commented May 24, 2017

oh ok. did you see the other issue #1 ?
basically there are 2 versions of gnuboy: the one that i call in my mind the "mob" version, which is what debian has in their repos. it is the community's continuation of laguna's project, which has some new features and changes. then there's this version which is basically the pure version of when laguna abandoned the project, plus some minor fixes to make it work on current systems. afaict, your work is based on the community/mob version; so maybe that one would be better suited to file a PR against.

@rofl0r
Copy link
Owner

rofl0r commented May 24, 2017

Regarding both the screencast and the snapshot functionalities, which could be an appropriate approach when trying to keep the program available for several distributions?

oh sorry, i overlooked the question in my previous reply.
what other (well-behaving) programs do in such a situation is to assume user wants all features, and then test if the required dependencies for each feature are available; if not, turn it off.
for example in case of feature "record video": a configure option --enable-ffmpeg which defaults to "auto" which means a compile test is done against ffmpeg.
this could either involve using pkg-config to check for its existence, or using gcc helloworld.c -lavcodec. if the latter succeeds it signifies that ffmpeg is installed; but then you also need to check for its headers, that would mean compiling a test program which includes a ffmpeg header such as:

#include <libavcodec/avcodec.h>
int main() { return 0; }

if the user passes --disable-ffmpeg, than all tests are skipped and ffmpeg support is not compiled in.
in the code this would look like

#ifdef WANT_FFMPEG
record_video();
#endif

@rofl0r
Copy link
Owner

rofl0r commented May 24, 2017

btw regarding the snapshot functionality:
there's already a screen buffer that has the entire bitmap that's put on the screen. what prevents you from just saving that to disk directly (for example using a .bmp header), instead of pulling in a dependency on scrot ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants