-
Notifications
You must be signed in to change notification settings - Fork 346
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
Use C99 and Makefile [need feedback] #21
Conversation
Built this locally, and I can say everything does work, including the makefile, even when using a mingw cross compiler, it has my seal of approval, but my approval sadly doesn't matter |
I can test building with mac tomorrow since I do have a hackintosh system. |
Makefile
Outdated
cd tables; python extract_resources.py ../$(ROM) | ||
|
||
clean: | ||
$(RM) $(OBJS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this also remove executable zelda3
($TARGET_EXEC
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and perhaps the tables/generated_*
files too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, I've added the $(TARGET_EXEC)
to the clean
target. Thanks
I've made a clean_obj
and clean_gen
both called by clean
.
This way, we can fine tune which part shall be cleared.
Current macOS support is tricky. macOS had separate installations for Python 2.x and 3.x. [System] Python2 has been fully removed on newer versions of macOS, but the python3 executable still resides at /usr/bin/python3. Example fresh-ish (this is my spouse's computer, I haven't done any setup with Xcode, brew, pynv, etc. that I usually do) install below:
The obvious local workaround is to replace |
All that being said, when the Python stuff isn't being a hassle, this PR works great. Here's my other machine with a working Brew-installed python in place of /usr/bin/python.
|
Pretty much my findings on my hackintosh, once I solved the python issue with brew, it compiled just fine, which is why I suggested a variable for python itself. Which should work on other systems where python 3 may not be default (which is hard to imagine nowadays) or don't rename python3 as python. |
Thanks for the feedbacks. I've switched I also tested on a rbp4 : the mono threaded DSP+PPU emulation make the game too slow for playing but no glitch was found. |
Makefile
Outdated
ROM:=tables/zelda3.sfc | ||
SRCS:=$(wildcard *.c snes/*.c) | ||
OBJS:=$(SRCS:%.c=%.o) | ||
GEN:=$(shell grep -hor tables/generated.*.h --include \*.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the grep command have a .
(for search location) at the end?
$(shell grep -hor tables/generated.*.h --include \*.c .)
Make is hanging after grep: warning: recursive search of stdin
on my machine.
Other than the one comment, changes seem to be working on my end 👍 |
I don't really like that you add struct in front of every struct name. Would you mind making typedef struct XYZ {} XYZ; instead? |
zelda_cpu_infra.c
Outdated
if (which & 256) { | ||
if (cmd == kSaveLoad_Save) | ||
return; | ||
sprintf(name, "saves/ref/%s", kReferenceSaves[which - 256]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the support for the saves/ref/ files?
main.c
Outdated
} | ||
|
||
void parseKeyconf(char* buf) { | ||
char* names[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this way of parsing yaml. Why pretend it's yaml when it's not...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. the file resembles an .ini file, would be easier to write and parse that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could rename it to .ini but it's also wont support ini features either
So what shall we keep ? txt ?
…e settings to file Merges PR snesrev#7 and some of PR snesrev#21 (without the move from C/C++ to C99): * Get SDL from Nuget * Makefile * Add F11 as Fullscreen toggle * Handle gamepad and keyconf.yaml mapping * Convert *.py to unix (LF, shebang, +x) Additionally: * Optional autosave on quit, autoload on start * Autozoom to best size for screen, or change zoom manually * Keep proper ratio in fullscreen * Save settings to config.yaml * In addition to key changes from PR snesrev#21 , also: Esc to quit Ctrl+P or Pause/Break to pause +/- to change zoom F11 or Alt+Enter to toggle fullscreen PgUp/PgDn/Home to navigate stages in saves/ref/
re: the previous comments about struct typedefs, should be resolved by https://github.com/yne/zelda3/pull/1 (@yne feel free to merge or patch manually) |
main.c
Outdated
@@ -52,7 +52,8 @@ int main(int argc, char** argv) { | |||
printf("Failed to init SDL: %s\n", SDL_GetError()); | |||
return 1; | |||
} | |||
SDL_Window* window = SDL_CreateWindow("Zelda3", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 512, 480, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch is not needed cause I added Alt+Enter for fullscreen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I was not aware of that change. I'll remove it.
- Tested with gcc, clang, tcc, chibicc - Makefile can handle asset extraction - Ported C++ Save/Load/Patch function in zelda_cpu_infra - Use struct typedef declarations (by @keatongreve) Co-authored-by: Keaton Greve <keatongreve@google.com>
I committed the C++ -> C transition. Please re-submit the other things. |
I just finished rebasing over our previous commit. This new commit give me even more conflict |
@snesrev : Impressive reverse, and impressive rebase :) The only missing part I noticed after rebase mine after yours:
Tell me if you are still interested for a PR for the makefile (the actual one is kinda bloated) |
I focused on the C++->C part only. The other things are still interesting but I wasn't sure how to merge your makefile with the current makefile. |
I'm also interested in some type of key remapping system. |
Decide the file extension and I'll send the PR ;) Note that I'll have to move all existing internal shortcut to Ctrl+* |
I could write an ini-style parser. I don't remember what it looked like but I don't like strstr for parsing. |
Reset / E is not important. |
Totally understandable. Also the crystal fix 70ba5d1 seems to also have fixed the poly render in intro animation (I guessed this animation style was specific to the US version) Makefile extraction PR : https://github.com/snesrev/zelda3/pull/31/files?w=1 |
Hm it doesn't use LTO = -flto=auto though... |
.PHONY: clean |
F11
fullscreen toggle (see: Fullscreen Support #22)Feedbacks :