Start documenting Distortion World and its camera

[Kuruyia]

This begins the documentation of overlay009, which is in charge of managing stuff related to the Distortion World.

For now, the scope of this PR has been limited to the following:

• start documenting structures used throughout this overlay, more specifically the DistortionWorldSystem struct
• start documenting the structures found inside the files of the tw_arc.narc NARC
• document functions and structures related to camera management inside the Distortion Word, including the templates found inside the tw_arc.narc NARC

This also documents the ScrCmd_31F map script command, that is only used in the Giratina Room map of the Distortion World to reset the camera angles that are present in the persisted map features data.

[ravepossum]

This is a real behemoth of a module, nice first chunk!

Overall thought I want to hear opinions on - would it be worth us picking some prefix to use for distortion world functionality e.g. DW? I'm usually not a fan of stuff like that since it reduces grep-ability, but in this case there's so much DW-specific functionality and preprending everything with DistortionWorld makes it pretty verbose. I'm kinda torn about it myself, but that just came to mind when looking at some of the function and struct names here.

[ravepossum]

thought: Should this be Persistent instead of Persisted (same applies to other usages throughout)? I think it's a little unclear what Data means too.

[Kuruyia]

Should this be Persistent instead of Persisted (same applies to other usages throughout)?

I started calling it "Persisted" throughout the code base, which includes other, non-distortion world related parts of the code.
If this is a case of bad English from my part then sure, we can rename this as part of a refactor ^^ It'll be a refactor that's not DW-specific tho'

I think it's a little unclear what Data means too.

I guess this is intended, as this "Data" refers primarily to a chunk of data present in the save data that maps can use to store whatever they need (documented here, and in the save data, this is this variable). So, by nature, this data is generic.

Even if, at the level of a single module, this naming may sound less clear, the intent here is to provide a consistent name throughout the code base for this part of the save data, which would allow better grep-ability and a quick way to understand what "PersistedData" refers to when encountering such a struct (as those structs that end up in the save data are quite spread out).

[ravepossum]

Sounds good. If I'm understanding what this does right, persistent definitely sounds more correct to me, but we could do a rename separate from this.

[Kuruyia]

Let's rename it later then.

[ravepossum]

question: Would this be nearly as clear as just ResetDistortionWorldCameraAngles?

[Kuruyia]

This script command calls the following function:

pokeplatinum/src/overlay009/ov9_02249960.c

Lines 1612 to 1622 in c15cb4f

	void DistortionWorld_ResetPersistedCameraAngles(FieldSystem *fieldSystem)
	{
	GF_ASSERT(fieldSystem != NULL);
	PersistedMapFeatures *persistedMapFeatures = MiscSaveBlock_GetPersistedMapFeatures(FieldSystem_GetSaveData(fieldSystem));
	DistortionWorldPersistedData *data = PersistedMapFeatures_GetBuffer(persistedMapFeatures, DYNAMIC_MAP_FEATURES_DISTORTION_WORLD);
	data->cameraAngleX = 0;
	data->cameraAngleY = 0;
	data->cameraAngleZ = 0;
	}

Which only acts upon the camera angle in the persisted data. I'm worried calling it ResetDistortionWorldCameraAngles would imply there is an effect that is immediately visible (although it may have such an effect indirectly, I didn't test).

Also, this script command is only called in map script scripts_distortion_world_giratina_room.s, so I don't think having a longer name has a huge impact.

[ravepossum]

Sounds good. Might be a good candidate to substitute in DW if we do want to abbreviate that across the board.

[Kuruyia]

Overall thought I want to hear opinions on - would it be worth us picking some prefix to use for distortion world functionality e.g. DW? I'm usually not a fan of stuff like that since it reduces grep-ability, but in this case there's so much DW-specific functionality and preprending everything with DistortionWorld makes it pretty verbose. I'm kinda torn about it myself, but that just came to mind when looking at some of the function and struct names here.

I don't think it would reduce grep-ability that much. You could grep for DW_ (with the trailing underscore) to get the results you want. Doing so currently only leads to two results:

❯ rg DW_
include/dw_warp/dw_warp.h
1:#ifndef POKEPLATINUM_DW_WARP_H
2:#define POKEPLATINUM_DW_WARP_H

I'm more worried about people not getting right away what "DW" stands for (Doctor Who?), but it's only a matter of learning once the acronym.

We could also use the TornWorld_ prefix, which is a certified based name. Certainly a less understandable one too.

[lhearachel]

I'm more worried about people not getting right away what "DW" stands for (Doctor Who?), but it's only a matter of learning once the acronym.

I worry about this, as well. What about DistWorld as a compromise?

[Kuruyia]

What about DistWorld as a compromise?

Sounds good to me.

[ravepossum]

My bad, this slipped off my radar - re: the DW/DistWorld/etc. shorthand, one of the primary things I was concerned about the size of was the name of structs and their associated functionality - e.g. DistortionWorldFileCameraAngleSection and it looks like you've only used it for the module prefix. (as an aside, I think I still prefer something like DW over DistWorld to really commit to saving space but that is by no means a hill I would die on).

Beyond abbreviation concerns, I think I would rather us apply the shorthand universally rather than just when using it as a module prefix or not at all - applying it some of the time feels like it adds some inconsistency that may make the Distortion World-related code slightly harder to traverse.

I don't want to hold this PR up much longer though - we can also revisit the naming in future PRs. I just wanted to avoid ground-retreading in future Distortion World PRs since it'd be nice to stay consistent.

[Kuruyia]

My bad, this slipped off my radar

No problem, life got (and still gets) on my way for me ^^ I'll need to take some time to properly get back to this PR and module.

the DW/DistWorld/etc. shorthand, one of the primary things I was concerned about the size of was the name of structs and their associated functionality

Agreed, I just pushed an update to the name of the structs. Shortened the names to DistWorld for consistency with the prefix that was already chosen for the function names.

Beyond abbreviation concerns, I think I would rather us apply the shorthand universally rather than just when using it as a module prefix or not at all - applying it some of the time feels like it adds some inconsistency that may make the Distortion World-related code slightly harder to traverse.

Agreed. We still have some DistortionWorld in the codebase, do we want to change all of them to DistWorld/DW?

Those appear mostly in the middle of function names (e.g. LandDataManager_DistortionWorldX, SystemVars_XDistortionWorldY, SystemFlag_XDistortionWorldY...). There's also the DistortionWorldWarp struct.

I don't want to hold this PR up much longer though - we can also revisit the naming in future PRs. I just wanted to avoid ground-retreading in future Distortion World PRs since it'd be nice to stay consistent.

Yep, let's ask ourselves the question later on once the module is more documented. Having a better vision of what this module interacts with may help.

I'll rebase this PR and fix conflicts once it is greenlit.

[lhearachel]

Yep, let's ask ourselves the question later on once the module is more documented. Having a better vision of what this module interacts with may help.

Sorry for my lack of response here; I think that DistWorld is pretty reasonable here, given that it sensibly maps to DistortionWorld within the repository's context and is hard to mentally map onto some other concept once that link is made. Let's go with that one. 👍

[ravepossum]

Apologies again, still quite busy. The newest set of changes look good to me. Should just need to sort out the conflicts and we'll call this one good.

[Kuruyia]

Should be good to merge :)