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

3.9 #248

Merged
merged 306 commits into from
Sep 16, 2021
Merged

3.9 #248

merged 306 commits into from
Sep 16, 2021

Conversation

OctoFlareDev
Copy link
Collaborator

@OctoFlareDev OctoFlareDev commented May 23, 2021

Fixes issue #214.
Fixes issue #198 (sort of).

@OctoFlareDev OctoFlareDev changed the title Displays Original Author while no Author Adds / Fixes Various Stuff May 23, 2021
@OctoFlareDev
Copy link
Collaborator Author

OctoFlareDev commented May 24, 2021

Ready to merge.

(Really right now)

@Bentroen
Copy link
Member

Nice!! Thank you for fixing the stuff I pointed out. It's now much better overall :)
There are still some things I'd like to polish before merging. For the sake of completeness, I'll add them to this pull request over the next few days, instead of merging right away. (I need to acquire the habit of using one pull request per feature anyway, haha!)

@Bentroen
Copy link
Member

Bentroen commented Jun 6, 2021

I really like how the changes are looking so far! Here's a few additional improvements I'd suggest:

  • I see you added a 'Back' arrow on the Preferences. We should keep the OK button at the bottom though. Themes should (mostly) only change the appearance and not where you must click to perform a certain action.
  • I like the idea you're trying to convey with the layer lock icon; however, I'd still rather go with a padlock since the feature is called 'layer lock'. Also, it looks too similar to the volume icon and is not consistent with the existing themes.
  • Having duplicate code for the Preferences window is a bit cumbersome, making it difficult to maintain. In one instance, specifically the 'Check for updates' toggle, there are now eight similar but slightly different lines responsible for drawing it — when a simple rearrangement in the code layout could keep it to a single one. I'll rework the Preferences a bit and try to fix this!
  • The border surrounding the note block area is a bit off — there's a 1px gap between both the marker head and the bottom scrollbar. (Perhaps it could also use a softer color?)
  • Bold font is missing in some places, like the titles in each released version's list of changes.
  • When hovering web links, the font becomes gray. A lighter shade of blue would probably look better.
  • The tempo edit window could be opened by clicking the tempo box instead of from the Edit menu. The menu option could also be removed.
  • Entering a non-numeric value on the tempo causes an issue.
  • Edge scrolling when dragging a selection is faster depending on FPS.
  • Sound count is updated far too often on higher FPS.

I'd really like to ship a dark version of the Fluent theme as well. Let me know if you'd be able to work on it; otherwise, I'll give it a shot. :)

@OctoFlareDev
Copy link
Collaborator Author

It's better to test working and useable before committing to avoid conflicts

@OctoFlareDev
Copy link
Collaborator Author

About the url text, the Fluent design format shows that when hovering on URL or hyperlinks, the text becomes gray.

@OctoFlareDev
Copy link
Collaborator Author

merged my experimental branch into this because why not and also no more conflicts

Ctrl + P --> Preferences
F7 --> Cycle through the FPS options and display a popup
@OctoFlareDev
Copy link
Collaborator Author

Alt + Enter to fullscreen like most games
Can be useful for recording on 1080p screens

@Bentroen
Copy link
Member

Since Realtime Blur, the extension used for the Acrylic and Mica effects, is a paid extension, it wouldn't be ethical to publish its source code on the repository, as this would enable anyone who cloned the project to obtain the extension for free.

In order to remove its source code from the repository, I did an interactive rebase editing commit 89f3da4 (new hash) to remove the affected files. During development, these files were added to .gitignore (d08a9cd) and eventually deleted (cdd4017 -- of course this commit no longer removes any files because they're now gone, so the commit message no longer makes sense). However, without the cleanup, the files would remain in the repository history, which would still allow access to the source code.

To allow the project to work even with the extension files missing, commit 634b383 introduced a script, blur_scripts_alt.gml, which acts as a wrapper to the functions defined in the extension. This way, when the files are absent they just return placeholder values that allow the program to run without the blur effect, but still showing transparency. Simply deleting the files would cause the program to crash otherwise.

This way, anyone who clones the repository will be able to run the program as normal. However, in order for the blur effect to work, a copy of the extension must be placed in the correct spot. For production releases, whoever builds the version must make sure to load the files correctly (this will likely be me, so consider this a note to self :D). For record, a list of the affected files is kept below:

Affected files:
shaders/blur_realtime/blur_realtime.fsh
shaders/blur_realtime/blur_realtime.vsh
shaders/blur_realtime/blur_realtime.yy
shaders/blur_static/blur_static.fsh
shaders/blur_static/blur_static.vsh
shaders/blur_static/blur_static.yy
scripts/blur_scripts/blur_scripts.gml
scripts/blur_scripts/blur_scripts.yy

One consequence of rebasing is that it rewrites the branch's history, which requires a force push. So if anyone besides me and @chenxi050402 has based work off this branch, make sure to clone it again!

With that, this PR should be (finally) ready to merge into the project! 🥳

That fix somehow got lost in the merge conflicts.
- Scrolling while pressing down the mouse wheel is uncommon and sort of difficult.
- Ctrl + Scroll wheel is a widely used shortcut for zooming in/out, so many users won't need adaptation.
- If a user needs to scroll the song while selecting notes, they may use the scrollbar or release Ctrl temporarily. Also, it seems more appropriate to drag a selection box instead of selecting individual notes if notes outside the viewport must be selected.
@Bentroen Bentroen merged commit dc345f1 into OpenNBS:development Sep 16, 2021
@OctoFlareDev OctoFlareDev deleted the development branch September 16, 2021 08:06
@OctoFlareDev OctoFlareDev restored the development branch September 16, 2021 10:03
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.

3 participants