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

NSF Driver and Export Fixes #262

Merged
merged 41 commits into from
Jan 2, 2025
Merged

Conversation

Gumball2415
Copy link
Collaborator

@Gumball2415 Gumball2415 commented Feb 17, 2024

This pull request aims to fix various bugs regarding NSF driver export and execution.

This pull request improves upon and supercedes #156.

Changes in this PR:

Leafuw, nyanpasu64, and Zeta, thank you for your help!

@Gumball2415 Gumball2415 force-pushed the app-emu-module-nsf_driver-dev branch from 9f54952 to d5a976c Compare February 17, 2024 18:28
@Gumball2415 Gumball2415 force-pushed the app-emu-module-nsf_driver-dev branch from 39cb8f4 to 6f2c743 Compare March 22, 2024 17:57
@Gumball2415 Gumball2415 force-pushed the app-emu-module-nsf_driver-dev branch from 9e937ab to d92a3a1 Compare July 19, 2024 14:16
@Gumball2415
Copy link
Collaborator Author

TODO: fix linear pitch pitch sequences

fixed in 4ce047d

TODO: fix asm export DPCM start address issue with beep.dnm

fixed in fa05bd9

Thanks, Leafuw and nyanpasu64!
@Gumball2415 Gumball2415 marked this pull request as ready for review September 22, 2024 10:16
@Gumball2415 Gumball2415 changed the title NSF Driver Fixes NSF Driver and Export Fixes Sep 22, 2024
@Gumball2415 Gumball2415 marked this pull request as draft October 5, 2024 15:43
@Gumball2415 Gumball2415 force-pushed the app-emu-module-nsf_driver-dev branch from 893e3ec to 81be243 Compare December 25, 2024 21:00
@Gumball2415 Gumball2415 force-pushed the app-emu-module-nsf_driver-dev branch from 8baa876 to 3c3f1a3 Compare December 25, 2024 22:11
@Gumball2415 Gumball2415 marked this pull request as ready for review December 27, 2024 14:46
@nyanpasu64
Copy link
Collaborator

  • is ChunkRenderText used for assembly export? i have not tested it, do you know it works for generating nsfs or games?
  • // str.Format("; Bank %i\n", pChunk->GetBank()); generally when making a PR i usually delete code or clarify why it's not needed, instead of commenting out (which can confuse the next reader what the code is there for)
  • i do not know enough of the code to provide meaningful feedback
  • what is pChunk->StoreByte(m_iWaveCount);? does this allow the runtime driver to bounds-check an instrument? seems the driver was modified to take into account, at comment check if within actual wave count.
  • PatternCompiler.cpp is giving me flashbacks to (invalid) notes popping up in 0cc 0.3.15.x when it corrupted its own memory
    • did you both turn Print into a template that calls _sntprintf_s internally (the more characters you put around printf the more powerful it is 😉), and only call it with CString::Format() single strings? in theory this would be dangerous if a % sign pops up in the string and the sprintf-like call thinks it's a format specifier and corrupts memory instead, but in practice it seems you only call the function with error messages populated with just integers.
  • have not looked through the pattern editor code, seems you replaced (invalid) with question marks?

@Gumball2415
Copy link
Collaborator Author

Gumball2415 commented Jan 1, 2025

responses in the same list order:

  • CChunkRenderText handles writing the module data into a CA65 assembly representation (much like how CChunkRenderBinary handles writing module data into compiled PRG bytes for .bin)
  • noted, will amend this.
  • not much comment, other than this is also a good reason why documentation should be improved
  • this stores the amount of waves an N163 instrument contains to help with bounds checking when indexing wave data. this was commented out in the source code of both the tracker and NSF driver as early as vanilla 0.4.0.
  • i forgot why i changed it like that, maybe just a hack for formatted string printing?
    • edit: this implementation is directly copied from ExportDialog.cpp. all it does is replace LF line endings with CRLF for MFC.
    • this will be part of an eventual goal to substitute CString with std::string whenever possible (Replace CString with std::string #258)
  • i changed it from printing (invalid) to ? to fit the text within each note, instrument, volume, and effect command.

- Use `string_view` type instead of `LPCTSTR` for log writing
  - Addresses #258.
- Format text with indenting
- Print diagnostic messages normally not seen in release
@Gumball2415 Gumball2415 merged commit a9cbf00 into main Jan 2, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment