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

[Unfinished] Some patches #1213

Closed
wants to merge 6 commits into from
Closed

[Unfinished] Some patches #1213

wants to merge 6 commits into from

Conversation

Buggem
Copy link

@Buggem Buggem commented Nov 22, 2024

Changes:

So far:

  • Set FS permissions from 777 (bad, because it gives rw permissions to every person, even low level users) to 755, the better permission
  • Added text L+R scrolling to the Script Editor

...and more that I am planning to add in the future:

  • Multiplayer(?)
  • Looking around for bugs
  • OmniScale (some code from SameBoy, the license is pretty safe, only if there is not a substantial amount of code from the project)

Legal Stuff:

By submitting this pull request, I confirm that...

  • My changes may be used in a future commercial release of VVVVVV
  • I will be credited in a CONTRIBUTORS file and the "GitHub Friends"
    section of the credits for all of said releases, but will NOT be compensated
    for these changes unless there is a prior written agreement

@NyakoFox
Copy link
Contributor

Your changes should probably be in separate PRs.

Added text L+R scrolling to the Script Editor

I wrote #955 which has various text input improvements, including moving the cursor left/right

Multiplayer(?)

That's uh, quite an ambitious addition, maybe some discussion would be best first.

OmniScale (some code from SameBoy, the license is pretty safe, only if there is not a substantial amount of code from the project)

In my opinion, upscaler shaders have never looked good, and OmniScale doesn't seem like an exception... personal opinions aside, I don't think shaders make sense in the base repository, especially since we don't use shaders anywhere else yet.

@Daaaav
Copy link
Contributor

Daaaav commented Nov 23, 2024

Hi! So to add a few more things:

As Nyako said, there are multiple different changes in this PR, and it's better to make one PR per (related) change. Additionally, you may want to squash/fixup the commits a bit - the 1st commit adds a constant that's only defined in the 2nd one, which has a syntax error which is resolved in the 4th one, etc. Ideally, every individual commit can be compiled, and has a message describing the change (instead of "Some polish"). Don't make the first line of the commit message longer than 72 characters though. Or any further lines technically, but especially the first 😛

777 (bad, because it gives rw permissions to every person, even low level users) to 755, the better permission

777 is often seen as a "dangerous" permissions value because it gives permission to everyone to do everything, but that's not really the case. That low-level user also needs access to every directory along the way, including your home directory. If your entire home directory is already open for every user to access, restricting access to your VVVVVV directory is the least of your worries.

@Buggem
Copy link
Author

Buggem commented Nov 23, 2024

@Daaaav
I thought it would create a lot of clog to open too many pull requests, and at the time of adding the individual commits, I didn't have access to git and was using the GitHub UI.

@flibitijibibo
Copy link
Collaborator

These can be reviewed separately once they've been cleaned up and finished - nothing in here is horrible but it's nicer to get emails for PRs that are finished (or at least a complete draft).

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.

4 participants