Document pass on WaterBox code#2695
Conversation
|
Contributions past, current, and future made to this repository are licensed under CC0. I couldn't find the thread, i'm in a rush. |
| #define COLPOLY_VTX(vtxId, flags) ((((flags) & 7) << 13) | ((vtxId) & 0x1FFF)) | ||
|
|
||
| // flags for flags_vIA | ||
| // flags for variable flags_vIA |
There was a problem hiding this comment.
did you mean "struct member" or just "member" instead of "variable" (I think it was fine as is in main)
| /* The true purpose of the flag is unknown. The state is never enabled on any waterbox, and functions that | ||
| * pass on flag enabled are never called, so there is no direct usecase context. */ | ||
| #define WATERBOX_IS_DISABLED (1 << 19) // Disables waterbox collision |
There was a problem hiding this comment.
where does the name come from then?
There was a problem hiding this comment.
So in functions BgCheck_GetWaterSurface and BgCheck_FindWaterBox, the search routines will skip over a WaterBox with the flag set, and since these are the only two functions in use that process WaterBoxes, the flag basically "disables" them. WATERBOX_IS_DISABLED makes sense here.
However, my concern is that WATERBOX_IS_DISABLED could potentially be incorrect based on the context in which it's used in func_800425B0.
func_800425B0 has the same arguments as BgCheck_GetWaterSurface and performs the same search, except the one difference is that it now only selects WaterBoxes with the flag set. To me it's a really odd design and makes me believe that there was some other purpose for this flag. Like, one potential possibility that I have zero evidence of would be to have a flag turning water into lava, as those two things would be something that could share the same basic properties (e.g. flat, you'd want to know the surface height) but you would want to exclude from one another.
So in short, I opted to choose a name that make sense within the scope of what we know now, but added a comment to highlight that the assumption could potentially be incorrect, because again, never used.
There was a problem hiding this comment.
I see. The comment game me the wrong impression that the flag was entirely unused outside of unused code
Maybe:
| /* The true purpose of the flag is unknown. The state is never enabled on any waterbox, and functions that | |
| * pass on flag enabled are never called, so there is no direct usecase context. */ | |
| #define WATERBOX_IS_DISABLED (1 << 19) // Disables waterbox collision | |
| /* The original intended purpose of this flag may not be disabling a waterbox. See func_800425B0 (unused) which by contrast only considers waterboxes with this flag set. */ | |
| #define WATERBOX_IS_DISABLED (1 << 19) // Disables waterbox collision |
+ formatting
| * `outWaterBox` returns a pointer to the WaterBox | ||
| * | ||
| * This performs a special case check on a custom waterbox in Zora's Domain that has a finite depth | ||
| * Otherwise, the search performed assumes waterboxes cannot overlap, as water has effectively infinite depth |
There was a problem hiding this comment.
I don't understand the relation between not overlapping and having infinite depth
| //! @bug: WaterBox bounds check issue. | ||
| if (waterBox->xMin < x && x < waterBox->xMin + waterBox->xLength) { |
There was a problem hiding this comment.
did you forget to add details here?
There was a problem hiding this comment.
Yep, I only added it in MM woops.
Dragorn421
added
the
Waiting for author
2 participants
As I was attempting to document the newly found waterbox bug, I found a few mistakes and improvements to make in my old documentation.
I've implemented a similar pr in MM, and would like there to be a consensus reached between both games before merging.
The sister PR can be found here:
zeldaret/mm#1860