Conversation
|
Sorry to say this for such a large PR- but before looking I have to call out- if you're going to rename the lanterns, you have to make a matching PR against Archipelago and rename all of those too; while it's good to get logic improved, I might suggest leaving just the names as-is for this pass so that you don't have to open an AP PR just yet. That would reduce scope to only the lanterns with logical gaps, making this much easier to review and then we can handle the lower stakes name changes in both repos as a much quicker review. (I would also suggest not reordering the lanterns in code for this reason, as it makes the diff more complicated and gets the ordering in the file even further from our fixed 1337xxx ids in AP that we cannot change) |
|
Commenting here so discussion is not locked in the Discord server: Agreed to not reorder the lamps in this file, but the renaming is needed and an AP PR was going to be needed to sync the logic anyway. Will be making an AP PR as soon as I can manage it and I don't expect this to be merged until that PR is created. |
| Add(new ItemKey(1, 7, 856, 95), "Lake Desolation: Metropolis Bridge Left Lantern", null, LakeDesolationRight & LanternCube); | ||
| Add(new ItemKey(1, 7, 1048, 95), "Lake Desolation: Metropolis Bridge Middle Lantern", null, LakeDesolationRight & LanternCube); | ||
| Add(new ItemKey(1, 7, 1240, 95), "Lake Desolation: Metropolis Bridge Right Lantern", null, LakeDesolationRight & LanternCube); | ||
| Add(new ItemKey(1, 7, 1432, 95), "Lake Desolation: Metropolis Bridge More Right Lantern", null, LakeDesolationRight & LanternCube); |
There was a problem hiding this comment.
As a broad statement that I'm using these seven as an example of-
I question whether blanket getting rid of numbers actually makes these names any more intuitive.
Like, all of these lanterns fit within two screens, you're going to hit them all at the same time, do we really need to iterate out Leftmost, More Left, Left, Center, Right, More Right, Rightmost when 1-7 would suffice.
That wasn't a case where I arbitrarily put down numbers; I just intentionally numbered them left to right where you would see them.
in the end I don't mind too much this change; but generally when you mentioned clarifying names I thought it would be more like "1-4" becomes "above water 1-2" and "underwater 1-2"
the name changes are fine but they only really needed to be more granular when it wasn't obvious where one of them was.
There was a problem hiding this comment.
Because the numbering convention couldn't be consistent. I have two examples. First, this room. You could first enter this room from either side. So you could often be going backward in number order if we always number left to right. Ok, fine.
Second example: Backer Room. You always enter this room from the right. How do we number it? Left to right to be consistent with other rooms? Right to left because you're always moving through the room in that direction?
I quickly determined that the numbers just made things more confusing, always. There's no reason for them. So, to be consistent, I removed them everywhere that wasn't describing where it is (e.g. Varndagroth Tower (Left): Floor 2)
There was a problem hiding this comment.
And even if I had left them with numbers, there was a specific request NOT to reorder the code lines. Now the numbers would have been a chaotic mess in the code. Finding what you're looking for would be nigh impossible. The landmark system makes it easier to find, both for player and developer.
| Add(new ItemKey(11, 36, 254, 176), "Lab: Coffee Right Lantern", null, MainLabFlooded & LanternCube); | ||
| Add(new ItemKey(11, 16, 504, 169), "Lab: Sentry Left Lantern", null, (SeedOptions.LockKeyAmadeus ? UpperLab | (MainLab & R.LabGenza) : LabResearchWing) & LanternCube); | ||
| Add(new ItemKey(11, 16, 696, 169), "Lab: Sentry Right Lantern", null, (SeedOptions.LockKeyAmadeus ? UpperLab | (MainLab & R.LabGenza) : LabResearchWing) & LanternCube); | ||
| Add(new ItemKey(11, 35, 120, 1209), "Lab: Main Shaft Research Level Left Lantern", null, MainLabFlooded & LanternCube); |
There was a problem hiding this comment.
"Research" is not a term players know, it's my personal shorthand for the mini logical zone between the lasers in the left and the dynamo room.
I would just say Main Shaft "Bottom" or "Base"
| Add(new ItemKey(11, 35, 120, 537), "Lab: Main Shaft Genza Level Left Lantern", null, MainLab & LanternCube); | ||
| Add(new ItemKey(11, 35, 120, 905), "Lab: Main Shaft Lower Trash Level Left Lantern", null, MainLabFlooded & LanternCube); | ||
| Add(new ItemKey(11, 35, 264, 169), "Lab: Main Shaft Exp. 13 Level Right Lantern", null, MainLab & LanternCube); | ||
| Add(new ItemKey(11, 35, 280, 1209), "Lab: Main Shaft Research Level Right Lantern", null, MainLabFlooded & LanternCube); |
There was a problem hiding this comment.
Same comment there
| Add(new ItemKey(3, 13, 251, 125), "Forest: Ramparts Bridge Lantern 2", null, RefugeeCamp & LanternCube); | ||
| Add(new ItemKey(3, 13, 747, 125), "Forest: Ramparts Bridge Lantern 3", null, RefugeeCamp & LanternCube); | ||
| Add(new ItemKey(3, 1, 235, 125), "Forest: Lantern Past Signpost", null, RefugeeCamp & LanternCube); | ||
| Add(new ItemKey(3, 12, 1019, 109), "Forest: Rats Top Lantern", null, RefugeeCamp & LanternCube); |
There was a problem hiding this comment.
I don't see any other lanterns in room 3, 12; what is the Top distinguishing it from?
There was a problem hiding this comment.
I added Top here because I was personally having trouble trying to figure out where the lantern was in the room.
The thing for both is that the numbers represented clusters; lanterns that had the same logical access as each other within the same room; you'd usually be collecting them within seconds of each other; no real player need for which is which because it's too minuscule to affect routing, and no real developer need because if someone says "lantern 3 is bugged" you know the coordinates.
the request was more that you can't change all the variables and position at the same time, because then it becomes a full file rewrite; the amount that some of these changed already suppressed diff highlighting for logic changes in places like Metropolis C-Card, which made it more likely for things to slip through the ask there was more to just do the handful of important logic changes first and then any renaming/reordering for less important internal housekeeping after, but I was fine compromising to just not re-ordering if you wanted to do them both at once. the desire there was to get the important 15-or so clusters with logic changed isolated so they wouldn't have the noise of having several hundred non-impactful renames happening at the same time but I don't want to split hairs; the more verbose and explicit names do give a bit more specificity which can be helpful in rare cases. anyways, finished the pass at the names, the names look fine. taking a look at logic now. |
| Add(new ItemKey(2, 32, 120, 192), "Varndagroth Towers (Left): Bottom Floor Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 34, 232, 1520), "Varndagroth Towers (Left): Elevator-Only Floor Lantern", null, MidLibrary & R.CardE & LanternCube); | ||
| Add(new ItemKey(2, 34, 232, 2512), "Varndagroth Towers (Left): Elevator Bottom Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 43, 200, 192), "Varndagroth Towers (Left): Stairs Base Left Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 43, 600, 192), "Varndagroth Towers (Left): Stairs Base Right Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 12, 168, 256), "Varndagroth Towers (Left): Stairs Floor 2 Left Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 12, 632, 256), "Varndagroth Towers (Left): Stairs Floor 2 Right Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 36, 312, 208), "Varndagroth Towers (Left): Stairs Middle Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 37, 152, 192), "Varndagroth Towers (Left): Ladder Room Left Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 37, 728, 192), "Varndagroth Towers (Left): Ladder Room Right Lantern", null, MidLibrary & LanternCube); | ||
| Add(new ItemKey(2, 39, 264, 192), "Varndagroth Towers (Left): Bridge Entrance Left Lantern", null, UpperRightSideLibrary & LanternCube); | ||
| Add(new ItemKey(2, 39, 552, 192), "Varndagroth Towers (Left): Bridge Entrance Right Lantern", null, UpperRightSideLibrary & LanternCube); | ||
|
|
There was a problem hiding this comment.
triple-checking this batch because unlike the rest of this file git isn't showing any part of the diff highlighted even with whitespace suppressed for these 12 lines
I see 2, 32 loses card C and the rest seem just to be renames? just making sure I'm not missing any logic masked by the renames
There was a problem hiding this comment.
This is correct and I don't know why the diff did this.
|
Confirmed so far- 9, 19 (Shroom Jump room); timestop removed; yes, none of these four lanterns need the ledge jump this room is named for 9, 8; 9, 14 Xarion hallways; swimming added; yes I can confirm rising tides floods these (sidenote no idea why my debug option to start with water mask got removed sometime over the last several months :( added it back locally) 11, 3; MainLab and need swimming condensed to MainLabFlooded; that's fine, doesn't change anything but is clearer. Even if MainLab is weird and synonomous with MainLabFlooded is I've been meaning to look at what the extent of main lab flooded is; that's a Jarno region that I kinda worked around when I added Lock Key Amadeus; I know it blocks you entering the region from the left, but yeah I think if you come from Genza you could technically get a few lanterns prior to swimming that the rando thinks you can't get to. MainLabFlooded = LabEntrance & R.CardB & NeedSwimming(FloodsFlags.Lab); |
|
Confirmed: 12, 24 - Emperor's Tower Giantess; changed from Tower (can reach gate + double jump or better) + infinite vertical to Tower Courtyard (can reach gate) + infinite vertical; one of the changes that is synonomous and doesn't change logic but is more straightfoward to read now 8, x - Caves of Banishment; Flooded removed from several rooms; Yes, anything below the shroom jump room seems to be flooded while everything above is not notably confirmed that the uppermost chests in 8,16 (waterfall) and 8,41 (jackpot) are reachable with a single jump off the water when the room is flooded. 16, 1 - Yes the top lantern in the right lower pyramid is above the water line, removing the swimming is correct 16, 11 - Yes, you do need double jump if you got the warp and are going back towards the shaft from the right, good catch 16, 24/25/9, yup those are in the right pyramid, not mid 16, 8 - yes if the shaft is flooded the secret room naturally is too |
|
Cross checking your PR description to confirm the last few: 3, 1 - Signpost lantern. yes that can be simplified to refugee camp 6, 27 - Yes the lower of these lanterns is reachable with nothing special 8, 31 - Yes, those are in upper caves of banishment |
TriumphantBass
left a comment
TriumphantBass
left a comment
There was a problem hiding this comment.
Okay, finished my reviews; I think this should all be fine.
Sorry to harp on the volume of changes so much; I just have to stress going forward that it's really important to group by severity and keep it in mind; broad sweeping trivial changes like renames are quick and easy to review, but they have to be under a lot more scrutiny when they're also housing critical bug fixes and logic changes.
It would have been a lot quicker to review a couple dozen lines of logic and then 500 renames than it was to keep checking that the 500 lantern changes that were mostly arbitrary renames weren't hiding more logic that I missed.
(a name that outright is wrong would be an exception to that and slightly more severe, like if "Spider-Hell" was labeled as "Coffee Break")
3 participants
Massive overhaul of Lantern Locations. What changed:
LeftSideForestCaves & LanternCube=>RefugeeCamp & LanternCube(This lantern is very clearly reachable from refugee camp, so it should be logiced over there)MidRoyalTower & LanternCube=>RoyalTower & LanternCube(This lantern doesn't require any extra verticality to reach, so if you can reach the warp gate you can reach this)CavesOfBanishmentFlooded & LanternCube=>CavesOfBanishment & LanternCube(Every single lantern in Caves of Banishment was marked as Flooded, which is simply not true)CavesOfBanishmentFlooded & LanternCube=>(FloodsFlags.Maw ? R.Free : R.DoubleJump) & LanternCube(This lantern in the upper right of Last Chance has been the most reportedly wrong lantern, now matches the last chance chest logic)CavesOfBanishmentFlooded & (MawGasMask | R.ForwardDash) & LanternCube=>UpperCavesOfBanishment & LanternCube(These are the two Mineshaft lanterns you pass on your way through Sirens' Caves, you don't need all this extra to get here)MidLibrary & LanternCube=>UpperLeftLibrary & LanternCube(These lanterns are on the upper library path and can't be reached unless you can also get to backer room)MidLibrary & R.CardC & LanternCube=>MidLibrary & LanternCube(This lantern is not inside the C card door, so take off the C card requirement)RightSideLibraryElevator & LanternCube=>LowerRightSideLibrary & LanternCube(This lantern at the bottom of the right elevator shaft can be reached from the bottom without E card)RightSideLibraryElevator & LanternCube=>UpperRightSideLibrary & LanternCube(Similarly, this lantern is accessible via the bridge from the left side, no E card required)SealedCaves & LanternCube=>SealedCaves & NeedSwimming(FloodsFlags.Xarion) & LanternCube(In Flooded Xarion, this lantern is underwater)SealedCaves & LanternCube=>SealedCaves & NeedSwimming(FloodsFlags.Xarion) & LanternCube(In flooded Xarion, this entire room is underwater)MainLab & LanternCube=>MainLabFlooded & LanternCube(While these two variables are currently equivalent, they should not be in the future. Lanterns that are fully underwater when the lab is flooded are marked with Flooded)EmperorsTower & R.UpwardDash & LanternCube=>EmperorsTowerCourtyard & R.UpwardDash & LanternCube(EmperorsToweronly marksDoubleJumpas a requirement overEmperorsTowerCourtyard, making these Courtyard reduces complexity slightly and makes the code slightly more readable)MidPyramid & LanternCube=>RightPyramid & LanternCube(These lanterns are clearly on the right side of the pyramid, mark them as such)MidPyramid & LanternCube=>RightPyramid & DoubleJump & LanternCube(These are also right side of the pyramid, but less obviously. Also,RightPyramiddoesn't inherently includeDoubleJumpbecause all the chests are lower than the warp point)MidPyramid & LanternCube=>MidPyramid & NeedSwimming(FloodsFlags.PyramidShaft) & LanternCube(This one was missing the flood flags)