Sync symbol name changes from pmd-sky

[ElectricGeorge]

Sync symbol names from pret/pmd-sky#52

[ElectricGeorge]

I was wondering about some of these things when I proposed these changes in the pret discord a while back. The main objection was the fact that many names have been in use by the rom hacking community for a while and changing them might cause confusion. In my opinion, as the decompilation progresses there may be namespace issues, so its a good idea to add prefixes. The strings in the prototypes indicate that the developers used "Pascal_SnakeCase" style function names where the prefix is the file name in Pascal case. FileRom_HandleRead is the actual name used in the original source for FileRead and it seems like there are several functions such as FileDevice_ReadStart that may cause confusion when investigated further. As for contributing, I don't see much of a problem with picking a name and then correcting it later if needed. I guess it could cause some confusion, but maybe this could be communicated in the documentation. I'm not familiar with all the goals of this project versus the pret decomp, but I think this warrants further discussion. As for No$GBA, I don't know of a solution and I didn't anticipate problems with it because I was using Ghidra. The way I see it, we are doing a matching decomp so we should try to match the original source as much as we possibly can with the information we have.

[theCapypara]

I agree with End, this has broken a few things in SkyTemple. I would please ask to not rename integral symbols like this without consulting other projects using pmdsky-debug.

Here's what I wrote on this issue on Discord:

@UsernameFodder Can we please come up with some stability guarantees about the pmdsky-debug symbols? https://github.com/UsernameFodder/pmdsky-debug/pull/236/files breaks SkyTemple, and I don't see why these name changes are neccesary. I will just stay on an old pmdsky-debug version for now. If I want to update I probably need to add some deprecated aliases to SkyTemple for some of those symbols since some may be used by existing patches. Additionally I am not really comfortable containing references to an illegal dump of a protoype ROM in my app. If pmdsky-debug doesn't want to make any guarantees I may need to come up with some way to automatically test compatibility of new releases... Do pmdsky-debug tools also detect name chnages for symbols? SkyTemple is complicated to maintain as-is and I'm for the most part the only maintainer so something like this is really annoying, and honestly I don't want to make it even more complicated by building checks like this. The only other option would be to detach from pmdsky-debug again in the long run, which won't really help the ecosystem or maintaining SkyTemple.

Regarding your points:

The main objection was the fact that many names have been in use by the rom hacking community for a while

The PMD Sky ROM hacking community has not used these terms so far. They have used what was previously in pmdsky-debug for at least 4 years now, with earlier documentation on some function and symbol names going back over 10 years.

As for contributing, I don't see much of a problem with picking a name and then correcting it later if needed. I guess it could cause some confusion, but maybe this could be communicated in the documentation. I'm not familiar with all the goals of this project versus the pret decomp, but I think this warrants further discussion.

The symbols are used by various ROM hacking projects, the main of which being SkyTemple and related tools for writing C and ASM patches for the game.

[theCapypara]

I don't have a problem with the renaming per-se, but there must be some mechanisms / processes in place to ensure some sort of stability with symbols, otherwise I think my only option is to fork the symbols and split the ecosystem again.

ROM hack & patch authors and tools need to rely on symbols names not changing or only changing in a controlled way (eg. after a deprecation period).

[theCapypara]

pmdsky-debug could be extended so that symbols could have deprecated aliases for example. Tools and integrations such as pmdsky-debug-py could then provide these aliases and emit deprecation warnings and pmdsky-debug could remove deprecated aliases after a while, let's say a year or two.

[ElectricGeorge]

I probably should have known it would affect SkyTemple, and I don't have a problem with rolling back these changes. Implementing deprecated aliases is a good idea, but in the meantime, syncing with the pret tool is not going to be an option. Maybe we can create a separate branch in the decomp repo for renaming and only merge into the main branch when we have a better solution over here.

[theCapypara]

It's not an immediate issue for SkyTemple, the releases are pinned to a specific version of pmdsky-debug, so I can just not update. So changes can stay in main from my perspective, if we find a solution later I can update.

[AnonymousRandomPerson]

The way I see it, we are doing a matching decomp so we should try to match the original source as much as we possibly can with the information we have.

As I mentioned in the pret Discord, my preference is having names that are descriptive and accepted by the hacking community. There is no requirement in a matching decomp to have the exact same symbol names as the real deal; what matters is that the raw bytes in the built ROM are the same, which symbol names have no bearing on.

If there is pushback on some of these symbols, I would prefer to rollback the changes and retain syncing capabilities rather than discontinue or complicate syncing just to use the authentic names.

[UsernameFodder]

I think this shouldn't have been merged so quickly.

I would please ask to not rename integral symbols like this without consulting other projects using pmdsky-debug. ... there must be some mechanisms / processes in place to ensure some sort of stability with symbols, otherwise I think my only option is to fork the symbols and split the ecosystem again.

Yeah my bad, in the future I'll leave PRs that rename stuff up for more time so people have time to object. I'm also open to tagging reviewers explicitly for renaming stuff if people want that.

Is this change really worth it? Why do we need to have the exact same names for functions? Is there other way of achieving whatever the goal of this change is?

I think this is somewhat complicated, and I agree with all the points everyone has raised to some extent, even though they're conflicting. I think moving forward, we'll likely have to brush up against the pret conventions if the decomp starts picking up steam, whether we like it or not. I'm not optimistic that everyone contributing to the decomp will be as considerate about pmdsky-debug as this change is, but labels in the decomp have too much potential to disregard trying to keep it synced with pmdsky-debug IMO.

For example, with the other syncing changes I'm currently working on (starting with https://github.com/UsernameFodder/pmdsky-debug/pull/230 and https://github.com/UsernameFodder/pmdsky-debug/pull/233, with more on the way), it's easier and IMO more consistent to follow the pret names (and by extension, the style in this PR, which is similar) in many cases, since a lot of our thus-far manually named system functions (now in arm9/libs.yml) are probably going to become surrounded by functions with the Pascal_SnakeCase convention, which is kind of weird. We could try to invent new names for every system function, but that would be pretty onerous, seeing as I'm hoping to mass-port over a bunch of them.

Ideally, for searchability, I think it would be great to include all the different names that a function can go by. That way it's more accessible to people from both communities. I think the proposal of aliases (see https://github.com/UsernameFodder/pmdsky-debug/issues/238) could be a good solution, and we can give priority to the existing (shorter) PMD-community names for the benefit of things like No$GBA. Assuming it's flexible enough to support everyone's different use cases.

[UsernameFodder]

Per the majority opinion, I think it could be good to rollback these changes temporarily, with the intention of adding them back as aliases or whatever other solution we end up agreeing on.

[AnonymousRandomPerson]

What's the point of including a prefix to each function? Is that really useful for reverse-engineering? Maybe it's helpful for the decomp, since functions are grouped in files there, but I don't see the point here.

For context, this isn't about files in the decomp, but rather the general idea of adding context to the function name in the style of a pseudo-class/namespace or denoting functions that work on a certain struct. I don't personally like this style either, but you'll see it in some C programming circles.

[ElectricGeorge]

@AnonymousRandomPerson I'm wondering if it is a good idea to revert main on pret as well and move the changes to a different branch (or my fork) since we are reverting the change here. This way further renaming work can be done without affecting the sync to this project. We can merge them back when we have a solution to this issue.

[AnonymousRandomPerson]

@ElectricGeorge A number of the prominent EoS hackers here don't prefer the Pascal_SnakeCase convention that the leaks use, so I'll probably revert to the old naming convention in the decomp. If the alias proposal goes through, we can add the leak names as aliases here, as that correlation is useful to have.

[mid-kid]

Though I have no bearing on the conversation aside from passive interest and having worked on decomps before, I really like the Pascal_SnakeCase scheme, as it helps group together "modules" of functions better, and is more conductive towards a consistent naming scheme, than anything I've seen pokemon decomps come up with in the past. I'd also try to stick to official SDK names for SDK things at least, as that's what the rest of the DS hacking and decomp scene uses, allowing us to have a single decomp "library" for the SDK in the long run.