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

Release OpenMSX 0.3.2 #1

Closed
wants to merge 6 commits into from
Closed

Release OpenMSX 0.3.2 #1

wants to merge 6 commits into from

Conversation

DevanWolf
Copy link
Contributor

@DevanWolf DevanWolf commented Apr 15, 2019

I decided to make a fixed and more compact version of OpenMSX as 0.3.2 which includes the following:

  • Update: Optimized MIDI data for all songs to reduce file size
  • Update: Replace 'Big Man Boogie' with fixed version of 'Keep on Rolling'
    And some more text file corrections

@Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Mar 2, 2021

This PR is problematic because tries to do many unrelated things at once (including bumping the release number!!!), which is probably why it has been ignored for years.

I suggest to split up this PR on a per-feature / per-bugfix basis. And drop the "release" portion of it entirely; there are other things that need fixing as well (see #7, #8, #9).

@nielsmh
Copy link

@nielsmh nielsmh commented Mar 2, 2021

Also, what exactly does this "optimized MIDI data" entail? It can mean a lot of things, and I'd like some documentation on what was actually done. (Sure I could go find/write a tool to compare the tracks, but that seems overkill.)

@Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Mar 15, 2021

I have more questions. It is about this:

Update: Replace 'Big Man Boogie' with fixed version of 'Keep on Rolling'

But why? What was wrong with the old track? What do you mean with “fixed version”? Is one of the current tracks broken?

@Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Mar 15, 2021

Okay, so have done some research on this. So it appears to me the problem this PR attempts to fix are real.
I have found an older attempt of DevanWolf to fix this here, with a better justification:

I noticed the MIDI data for all of the songs in OpenMSX have unoptimized data in them. Like the error found in tttheme2.mid the track with the jazz guitar in it is supposed to be channel 8 not 6, that I fixed. I decided to do this to help reduce file size. During this process, I realized during this MIDI editing process that 'Keep on Rolling' now plays correctly, so like "Replace 'Big Man Boogie' with fixed version of 'Keep on Rolling'" (Also changed source download link in readme file)

Source: https://dev.openttdcoop.org/issues/8623

I'm still not sure why this optimization is neccessary, but I am definitely not a MIDI expert, don't ask me!

Also, I have now figured out the rationale behind 'Keep on rolling', see #12.
I support using the 'Keep on rolling' track in OpenMSX now.

I suggest to do the following:

  • Close this PR as being too chaotic
  • Open new PRs to do the tasks cleanly separated, instead of doing everything at once. Specifically:
    • Bring back 'Keep on rolling' track (#12) in a new PR
    • Do the MIDI optimization stuff (if neccessary) in another PR
    • Text file cleanup is handled in #11
  • Review if this commit makes sense and make a PR if it does: d14bfa2

@Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Mar 17, 2021

The 'Keep on rolling' track has been included into OpenMSX!

Now the other major part in this PR is the optimization stuff. So I have no idea about MIDI, I am not an expert in this area, but it would be nice if someone more knowledgable about MIDI could look at our current MIDI files and check if there is any problem, and also check if this PR is doing things right.

@DevanWolf claims to have fixed something broken about OpenTTD journey:

I noticed the MIDI data for all of the songs in OpenMSX have unoptimized data in them. Like the error found in tttheme2.mid the track with the jazz guitar in it is supposed to be channel 8 not 6, that I fixed.

https://www.tt-forums.net/viewtopic.php?p=1220145#p1220145

It is unclear if the other files were “broken” as well, or if it was just a optimization for file size. :-/

So since this PR includes a possible bugfix, this is worth looking into.

@LordAro
Copy link
Member

@LordAro LordAro commented Mar 27, 2021

Going to go ahead and close this now, as most things have been superceded by others. Feel free to open new PR(s) for the relevant remaining things

@LordAro LordAro closed this Mar 27, 2021
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.

None yet

4 participants