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

Renamed Velocity to Speed Fix/#369 #561

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

ealvesss
Copy link

  • it was changed total of 27 files.
  • I'm using vsCode and it was creating a vscode profile folder, so I already added this folder in .gitignore file. Case it is a problem I can Separate the commits.

Check if it fits the spectation and let me know.

😄

(I didn't know which name should use in branch, so I put the number of the issue, hope its ok.)

@kemenaran
Copy link
Collaborator

Thanks for the PR!

As the discussion in the issue suggested, we actually settled on the inverse renaming: renaming everything from Speed to Velocity. The issue title remained the same, and so was misleading; sorry.

So the idea would be to do the inverse renaming (except maybe for cases where the symbol refers to an actual property of an entity, like the constant defining the speed at which it can move. But we just do a bulk rename for now, and catch the remaining tangential cases later).

@ealvesss
Copy link
Author

Oh @kemenaran so I missunderstood! The right way is the oposite! I will fix here then append the commit to the pr.

Sorry about that!

@ealvesss
Copy link
Author

The issue title remained the sa

I've searched in the project and it founded

  • 176 files
    • 2338 occurences

So, as you mentioned above, it would be better to use the "batch rename" approach and fix some specific points that we eventually find in the code.

p.s: I don't know if will be possible solve it today, but I will fix ASAP!

cheers 🎉

Copy link
Author

@ealvesss ealvesss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue title was wrong, so I missunderstood what I should rename to.

Now, it looks like was renamed properly (I hope so 😄 )

@ealvesss
Copy link
Author

@kemenaran fixed it. If I missed something just let me know! By the way, Happy New Year 🎉

Copy link
Collaborator

@kemenaran kemenaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few notes:

  • Sometimes the casing is wrong: "speed" (lowercase) gets replaced by "Velocity" (uppercase capital). You may want to replace "speed" with "velocity" and "Speed" with "Velocity"
  • Sometimes "speed" is replaced in a sentence belonging to a code comment. I think these should mostly stay the same than before. To help you may want to replace "speed" by "velocity" only if it is part of a symbol. A regex could help here: performing the replacement only if there's a non-space character just before or just after (so that "LinkSpeedX" gets replaced, but "adjust speed of music" doesnt).
  • It seems you added a "revisions/P0" directory by mistake :)

Otherwise it looks good to me. Thanks for your work, and happy new year!

@@ -35,3 +35,11 @@ DumpBanks
# save files
*.sav
*.ss*

### VisualStudioCode ###
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### VisualStudioCode ###
# visual studio code

Also, if you feel you can split this into another commit, that'd be great (but otherwise fine)

@@ -51,7 +51,7 @@ You can also read [disassembling How-Tos](https://github.com/zladx/LADX-Disassem
- [Artemis251's Link's Awakening Cache](http://artemis251.fobby.net/zelda/index.php): ROM map, maps data format
- [Xkeeper's Link's Awakening depot](https://xkeeper.net/hacking/linksawakening/): maps tilesets and save format infos
- [LALE](https://github.com/anotak/LALE): level editor, notes on maps data format
- [The Legend of Zelda Link's Awakening /DX Speedrunning Wiki](http://spiraster.x10host.com/LADXWiki/index.php/) : infos on wrong warps and map data format
- [The Legend of Zelda Link's Awakening /DX Velocityrunning Wiki](http://spiraster.x10host.com/LADXWiki/index.php/) : infos on wrong warps and map data format
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be replaced by "velocity"

; 1 = double-speed,
; 2 = half-speed
; 1 = double-Velocity,
; 2 = half-Velocity
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to keep "speed" here.

; Points to some data which somehow sets the music's speed
wMusicSpeedPointer::
; Points to some data which somehow sets the music's Velocity
wMusicVelocityPointer::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should stay to "speed" (it is music related, not entity-related)

@@ -2139,15 +2139,15 @@ def asRGBA8(self):
sample. This method is similar to :meth:`asRGB8` and
:meth:`asRGBA`: The result pixels have an alpha channel, *and*
values are rescaled to the range 0 to 255. The alpha channel is
synthesized if necessary (with a small speed penalty).
synthesized if necessary (with a small Velocity penalty).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replacement should not touch the scripts in tools/*


AnimateSlowWaterfallTilesGroup::
ld h, HIGH(AnimatedTiles) + $0 ; $1C22: $26 $6A

AnimateTilesSlowSpeed::
AnimateTilesSlowVelocity::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm… This is not related to an entity, but to an animation speed. IMO it should remain AnimateTilesSlowSpeed.

@@ -374,7 +374,7 @@ AnimateTilesMediumSpeed::
AnimateWaterCurrentsTilesGroup::
ld h, HIGH(AnimatedTiles) + $7 ; $1CD5: $26 $71

AnimateTilesFastSpeed::
AnimateTilesFastVelocity::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, it should remain AnimateTilesFastSpeed

@@ -6,21 +6,21 @@
; is transfered to address $100, which immediatly jumps here.
; (See header.asm)
Start::
; Switch CPU to double-speed if needed
; Switch CPU to double-Velocity if needed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CPU-speed is not entity-related: it should stay "speed"


.speedSwitchDone
.VelocitySwitchDone
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also, speed

; Takes a pointer to some data which determines the music speed.
macro set_speed
; Takes a pointer to some data which determines the music Velocity.
macro set_Velocity
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to music, not entity code, and should remain "speed"

@ealvesss
Copy link
Author

ealvesss commented Jan 8, 2024

OMG a lot of I will change! 😄

I will fix the pointed files ASAP.

Cheers!

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.

2 participants