Skip to content

[DLL] Remove redundant RUL2 adjacencies#493

Merged
memo33 merged 132 commits into
NAMTeam:stagingfrom
memo33:redundant-adjacencies-removal
Apr 28, 2025
Merged

[DLL] Remove redundant RUL2 adjacencies#493
memo33 merged 132 commits into
NAMTeam:stagingfrom
memo33:redundant-adjacencies-removal

Conversation

@memo33

@memo33 memo33 commented Dec 20, 2024

Copy link
Copy Markdown
Collaborator

For integration with the DLL-based RUL2 engine, this PR aims to remove a lot of RUL2 code made redundant by the adjacency handling of the DLL.

DLL-based adjacencies in a nutshell

Whenever you have two adjacent tiles A | B, the engine looks for an orthogonal surrogate tile S and matching RUL2 rules A | S ⇒ A' | S' as well as S' | B ⇒ S" | B' that make an override carry over from A to S to B. It then applies the result to the original tiles A | B directly, effectively applying a "virtual" rule A | B ⇒ A' | B'.

This means you only need to write RUL2 code that connects to pure orthogonals (in and out), but no adjacencies.

Diagonals work similarly, except they use two diagonal surrogate tiles. The effect is the same in that you only write RUL2 code connecting to pure diagonals (in and out).

(The orthogonal and diagonal surrogate tiles are chosen from a hard-coded list of IIDs.)

Goals of this PR

Help wanted! Especially the manual sighting is a huge task. It would be effectively open-ended, depending on how much of a clean-up we want this to be. The manual steps could even be skipped entirely without any cleanup (which shouldn't be less stable than the NAM 48 controller currently is).

@memo33 memo33 marked this pull request as draft December 20, 2024 17:25
@memo33

memo33 commented Dec 20, 2024

Copy link
Copy Markdown
Collaborator Author

2024-12-20_18 36 47

@jflann

jflann commented Dec 21, 2024

Copy link
Copy Markdown
Member

Nice job so far with all this.

I've largely re-written the SAM rule generator on the meta-sam branch with the NAM DLL in mind. It shouldn't produce any adjacencies and should be very close to fully functional and bug-free. I'm not sure the best way to integrate that work into this PR, but I think it's basically ready.

I'll still need to look over the RRW rule generator but I don't think there are many adjacency-related rules there to begin with.

@memo33

memo33 commented Dec 21, 2024

Copy link
Copy Markdown
Collaborator Author

Ah, thanks. For the meta-sam branch, would it make sense to use that code without the DLL as well? In that case, it could be merged into staging. Otherwise, we can merge it here eventually. It probably doesn't hurt to keep the branches separate for now till they're basically ready.

@TarkusSC4

Copy link
Copy Markdown
Member

Regarding 07_RHW/Sec7b_BaseNetwork and 08_NWM, I can confirm that most of the files contained in there are massive collections of adjacencies, and huge chunks can be pretty safely removed. There's likely only some select oddball things in those files that still require specialized code that isn't covered by the RhwRuleGenerator and the DLL's automatic adjacency handling, and most of them are going to be in the respective 00_Orthogonal and 02_Diagonal files. It'll require a bit of testing, but I have a fairly good idea of where to start with it.

@jflann

jflann commented Dec 21, 2024

Copy link
Copy Markdown
Member

Ah, thanks. For the meta-sam branch, would it make sense to use that code without the DLL as well?

No, I've made a lot of changes to the street network in light of the new diagonal functionality, so it only makes sense to merge for a post-dll version. I agree, we can just keep them separate.

@jflann

jflann commented Dec 22, 2024

Copy link
Copy Markdown
Member

RealRailwayRuleGenerator doesn't produce any adjacencies, so should be good to go.

@memo33

memo33 commented Dec 30, 2024

Copy link
Copy Markdown
Collaborator Author

I've updated the script so that it loads files in a deterministic order. Now the load order is the same as the one used by the controller compiler and the game. I've also restructured the Scala code a little bit. See the individual commits.

Finally, I reran the script (twice) from the original state of the RUL2 folder in order to make sure to discard any RUL2 changes made by previous versions of the script – as can be seen in the git history (branches/merges). I've discarded previous runs of the script using merge --strategy=ours.

The following diff compares yesterday's state of the RUL2 folder with the current one. In other words, this shows exactly the effect of switching to a deterministic load order. This way, even more redundant adjacencies were found:

Showing 484 changed files with 23,817 additions and 29,262 deletions.

328bb2e...a6c9d12

@memo33 memo33 force-pushed the redundant-adjacencies-removal branch from a6c9d12 to a654fa0 Compare December 31, 2024 16:22
@memo33

memo33 commented Dec 31, 2024

Copy link
Copy Markdown
Collaborator Author

I've rebased the branch (forced push) to factor out common functionality shared with #494.

@memo33 memo33 force-pushed the redundant-adjacencies-removal branch from a654fa0 to 0b0fb5a Compare January 1, 2025 19:01
@memo33

memo33 commented Jan 1, 2025

Copy link
Copy Markdown
Collaborator Author

Rebased on top of staging.

Interestingly, the removal of redundant adjacencies simultaneously eliminates a lot of conflicting overrides, not only in lines that have been removed, but also in lines that still exist. From the last commit:

Found 39173 conflicting RUL2 overrides (removed 37303, added 81).

@jflann

jflann commented Jan 4, 2025

Copy link
Copy Markdown
Member

I have started to look at some of the leftover adjacency files, mainly the RHW branch adjacencies. At least in the first few I've reviewed, the leftover rules (< 100, typ.) seem to fall in a few predictable categories:

  • Duplicate of foundational rules that exist in the FLEXRamp files
  • Adjacency for pieces which don't exist (either typos or not created yet, hard to know)
  • Rules which don't make sense (e.g., rules which require do a double switch)
  • Stuff involving RRW which isn't implemented, but if it were, would make these rules redundant.
  • OST code which overrides L1 versions of the RHW transition (which I'm guessing is a mitigation for some auto L1 behavior which may not be needed any longer.
  • A few good alt-GLR rules which could probably be made redundant by addressing the problem within the GLR codebase or the RHW generator.

Considering that, I think the best course of action for each of these files would be to verify that the foundational rules exist in FLEXRamp, then load up the remainder in the RUL Viewer to verify nothing else important is being deleted, then delete the entire file. If that's agreeable, I will start making changes.

@memo33

memo33 commented Jan 4, 2025

Copy link
Copy Markdown
Collaborator Author

Considering that, I think the best course of action for each of these files would be to verify that the foundational rules exist in FLEXRamp, then load up the remainder in the RUL Viewer to verify nothing else important is being deleted, then delete the entire file. If that's agreeable, I will start making changes.

I mostly agree. Though, I'm a bit worried by the amount of files, which can quickly get exhausting if you open them all in the RUL Viewer. If you see any patterns that allow eliminating larger chunks without having to manually go through the code, that could be helpful. For example, after adding rules for the alt-GLRs, rerunning the redundant-adjacencies script could eliminate some rules, although I guess that will cover just a small amount.

@jflann

jflann commented Jan 4, 2025

Copy link
Copy Markdown
Member

If you see any patterns that allow eliminating larger chunks without having to manually go through the code, that could be helpful.

Something that would help would be to delete parts of the code for height levels which don't exist. For example, RHW-2 only exists at L0-L2, but there are blocks of code in the ramp files for L3 and L4. This seems to be repeated across other networks as well. These are conveniently high-level subsections within the ramp adjacency files so they are easy to identify and delete. They seem to all begin and end with clearly identifiable block comments:

;-----------------------
;----L4 MIS (0x5742)----
;-----------------------

...

;------------------
;----END L4 MIS----
;------------------

@memo33

memo33 commented Jan 4, 2025

Copy link
Copy Markdown
Collaborator Author

Do you have a link to an example? I've done a quick search, but couldn't really find such blocks for networks other than MIS, RHW-4 and RHW-6S, which do support those height levels.

@jflann

jflann commented Jan 4, 2025

Copy link
Copy Markdown
Member

Do you have a link to an example? I've done a quick search, but couldn't really find such blocks for networks other than MIS, RHW-4 and RHW-6S, which do support those height levels.

Here are some examples. Maybe not as widespread as I was anticipating.

@memo33

memo33 commented Jan 5, 2025

Copy link
Copy Markdown
Collaborator Author

Ah, I see. I didn't know how the RHW ramp adjacencies were structured, but I think I get it now. What you describe is the following, right?

  • remove all the blocks for L3 MIS, L4 MIS, L3 RHW-4, L4 RHW-4, L3 RHW-6S, L4 RHW-6S using the format quoted above
  • from the files Controller/RUL2/07_RHW/Sec7b_BaseNetwork/Ramp* – excluding the following files, as these contain ramps that may support those height levels:
    • Ramp_{A,B,C,D,E}*
    • Ramp_4_*
    • Ramp_6S_*
    • Ramp_MIS_*

Does that sound right? What about non-ramp files?

Another idea would be to remove all rules that use IDs for which no path file exists. (Some intermediate dummy tiles don't have a texture or model, but a path file should always exist to circumvent lag while dragging.)

@jflann

jflann commented Jan 5, 2025

Copy link
Copy Markdown
Member

Yes, I think you've got it right. I think only the branch code is organized into network-specific files. The other ramp files have multiple networks in them.

Originally, I was thinking this would be pretty helpful to reduce the amount of code to look at, but I think this might be a distraction and instead we should try to focus on figuring out how the code is structured with the goal of removing entire files. This more than anything will really help to make things less overwhelming.

I'm wondering if maybe it would be easier to verify that the correct basic overrides exist in the FLEXRamp code and then just delete the corresponding ramp adjacency files? We know that these files contain adjacency involving top, bottom, or branch ramp tiles, so we just need to check that the special case (adjacency to base ortho or base diag) is intact elsewhere. In fact, this is where a script to check for identical rules would be helpful. Then, we could manipulate the loading order to ensure FLEXRamp loads before the ramp adjacency files, and tag/delete the duplicate lines in the adjacency. That would give us the confidence to just delete the whole file.

I also like the idea of removing rules for tiles that lack path files, but we'd have to be careful because I think it's fairly common for there to be "future-proof" rules created. Stuff like that should hopefully be commented out, but you never know.

@memo33 memo33 force-pushed the redundant-adjacencies-removal branch from 0b0fb5a to eb7520a Compare January 12, 2025 17:08
@memo33

memo33 commented Jan 12, 2025

Copy link
Copy Markdown
Collaborator Author

Rebased on top of #495 to fix the RHD/LHD file detection. This adds back a few rules that were previously detected as redundant adjacencies (since both RHD and LHD files had been loaded simultaneously). All in all, the changes between the previous state of the branch and the current branch are small though:

130 files changed, 8833 insertions(+), 6910 deletions(-)

With the driveside issue fixed, the redundant-adjacency checker finds about 5k rules that are redundant in RHD only. (Most likely this means that the rule in question should be RHD-only, or a LHD rule for the base ortho/diag override is missing or broken.) I left these untouched, as there are only a few of them. Chances are that removing entire adjacency files resolves many of them.

I'm wondering if maybe it would be easier to verify that the correct basic overrides exist in the FLEXRamp code and then just delete the corresponding ramp adjacency files? We know that these files contain adjacency involving top, bottom, or branch ramp tiles, so we just need to check that the special case (adjacency to base ortho or base diag) is intact elsewhere. In fact, this is where a script to check for identical rules would be helpful. Then, we could manipulate the loading order to ensure FLEXRamp loads before the ramp adjacency files, and tag/delete the duplicate lines in the adjacency. That would give us the confidence to just delete the whole file.

Yeah, I think this would be do-able, now that the RHD/LHD issues is fixed. My expectation would be that only a very small number of rules is duplicated across all the FLEXRamp files and that the base ortho/diag code exists separately.

Would you like to remove the files that you already identified as dispensable?

@memo33

memo33 commented Jan 13, 2025

Copy link
Copy Markdown
Collaborator Author

@jflann On branch duplicate-overrides (5000afb atop #495), I've updated the conflicting overrides checker, adding an option to also identify purely duplicate override rules (that aren't conflicting).

On the branch here, it finds about 280k duplicates, spread across the entire repository. Unsurprisingly, in many cases, the duplicate is equivalent to the rule directly above it, just using different orientations. LHD/RHD code is also prone to duplicating code if the code isn't split into driveside-specific files (which is a fairly recent addition of the controller compiler). By far the most duplicates are in the file Controller/RUL2/07_RHW/Sec7m_Crosslinks/Sec7m_Crosslinks.txt: 57826 duplicates of a total of 74050 override rules. Not sure what's going on there.

I'm debating whether it makes sense to run the script on the branch here and commit, just so that it provides more information, making it easier to delete something. Not sure. It would also increase chances of merge conflicts with independent RUL2 branches.

@jflann

jflann commented Jan 14, 2025

Copy link
Copy Markdown
Member

I'm debating whether it makes sense to run the script on the branch here and commit, just so that it provides more information, making it easier to delete something. Not sure. It would also increase chances of merge conflicts with independent RUL2 branches.

I can see why we might hesitate to do something that's pretty likely to cause a conflict. However, I'd consider the fact that the conflicts would just be comments, so resolving them in favor of the actual work would be a no-brainer. Plus, all the deleting we are doing is bound to cause some conflict anyway.

I just checked how badly conflicted my own feature branch, meta-sam, is with staging. There are some conflicts but it can still be quickly rebased. There might be others out there that would be more difficult to deal with. I think it will be good to have that info on this branch and worth the risk.

@jflann

jflann commented Jan 14, 2025

Copy link
Copy Markdown
Member

Actually, on second thought I've changed my mind. I'm afraid it might make too much of a mess. I've had a look at the results of such a scan locally and I'm surprise at the extent of the duplicate code. It is basically everywhere.

@TarkusSC4

Copy link
Copy Markdown
Member

By far the most duplicates are in the file Controller/RUL2/07_RHW/Sec7m_Crosslinks/Sec7m_Crosslinks.txt: 57826 duplicates of a total of 74050 override rules. Not sure what's going on there.

If the crosslink network's own RUL2 sections are set up properly, the similar bits in Sec7m_Crosslinks.txt should be redundant, as is the case with Line 811, which handles GLR. Sec10a_GLR1.txt has the exact same line at Line 2464.

0x57001700,0,0,0x08031500,1,0=0x57001700,0,0,0x5f880000,1,0

The other orientations of similar setups, i.e. where Diagonal GLR is extending out of a crossing with Orth RHW, don't have duplication in the GLR files, i.e., Line 3293:

0x57004700,2,0,0x08001a00,1,0=0x57004700,2,0,0x5f880600,3,0

But arguably, that kind of code really should be long GLR-side rather than RHW-side (i.e., in Sec10a instead of Sec7m).

It's also possible that some of the mid-adjacency code (i.e., to handle carrying a GLR override across multi-tile RHW network setups) might also be getting alternative coverage by virtue of the overrides to carry those RHW networks across GLR, too.

@TarkusSC4

Copy link
Copy Markdown
Member

Also, a thought on the whole notion of the former auto-L1/L2 RHW-2 coming out of HTs/OSTs . . . the big reason it was killed off was the insane number of overrides it was going to take. I'm honestly curious if the DLL's surrogate adjacency handling might allow that functionality to be re-enabled without having to accept the tradeoff of insane amounts of code vs. unacceptable instability. Based on how many questions we get from the general public as to why the auto-L1/L2 doesn't exist anymore, I kind of have the impression it's largely viewed as a necessary evil at best among our users.

@memo33 memo33 force-pushed the redundant-adjacencies-removal branch from 3d4375b to a3d9415 Compare January 14, 2025 18:50
memo33 added 11 commits April 19, 2025 20:13
This checks the alignment between the two output tiles if the can be
resolved using metarules.
The removals are primarily files that purely consist of plain adjacency
code that was previously extracted from metarules (recognizable by the
`;include` comments).

~124k lines of actual RUL2 removed.
These adjacencies are now redundant. The deleted code sections have been
removed in a structured way using the script:

    src/scripts/remove-rhw-adjacencies.py

The vast amount of deleted lines are comments (~2.3 million) vs ~130k actual lines of RUL2 adjacencies.
These adjacencies are now redundant. The deleted code sections have been
removed in a structured way using the script:

    src/scripts/remove-nwm-adjacencies.py

The vast amount of deleted lines are comments (~2 million) vs ~124k actual lines of RUL2 adjacencies.
@memo33 memo33 marked this pull request as ready for review April 20, 2025 20:49
@memo33

memo33 commented Apr 20, 2025

Copy link
Copy Markdown
Collaborator Author

The branch is in a solid shape now. Some of the recent changes:

  • removed the remnants of RHW and NWM adjacency code that are not needed anymore (about ~250k lines of RUL2 and 4.5 million lines worth of comments). This makes the entire repository significantly smaller and faster to work with.

  • implemented a ReverseResolver which maps an ID to its corresponding metarule tiles. In the sbt console, it can be used interactively using the preimage function:

    preimage(0x57294745)
    // val result = List(L2Rhw8c~(0,+2,0,-2) & Glr3~(3,0,0,1))
  • implemented a SegmentOrientationChecker script. It uses the reverse resolver to map RUL2 code from plain IDs to metarules where possible. It then runs several sanity checks on the metarule tiles and their segments in order to assert that there aren't any unexpected mismatches in direction or orientation. For example, if a rule accidentally reverses the direction of an ARD-3 segment or if the output tiles don't connect properly (in a loose sense), this would be detected.

    The script found roughly 100k buggy overrides so far. I've added the script to the GH actions, so it will prevent these kind of errors going forward. As more and more ID definitions are added to the metarule resolvers, the script will even get stronger over time.

  • manual tweaks of RUL2 code here and there. In particular, I've removed all prevents intended as auto-connect killers.

With that, I think the branch is ready for merging and inclusion in the next build. Note that I've already merged the meta-sam branch here. I've also done some testing already, but getting more people to test, especially the more obscure features and edge cases, will be essential to be able to find and fix any remaining issues going forward.

One of the parts of RUL2 that's not quite in shape yet is the Viaducts code. There aren't enough comments to identify the adjacencies in there, so I'm inclined to leave the code as is, for now. Most likely, it will eventually be fully replaced by metarules, anyway. Even just adding more of the Viaduct IDs to the metarule ID resolver will help detect many buggy overrides, by means of the SegmentOrientationChecker script. That can wait for a new PR though.

@memo33 memo33 force-pushed the redundant-adjacencies-removal branch from 9c11528 to b885acd Compare April 23, 2025 17:21
@memo33

memo33 commented Apr 26, 2025

Copy link
Copy Markdown
Collaborator Author

I've made a few more tweaks. In particular, I went ahead and implemented the Viaducts IID scheme as a resolver, which allowed me to identify some more buggy overrides in the Viaducts RUL2 code. Now I'm done here.

@jflann

jflann commented Apr 27, 2025

Copy link
Copy Markdown
Member

Truly a massive amount of hard work on this branch. I am excited to see the results integrated into our next build. I'm good with merging this now.

@memo33

memo33 commented Apr 28, 2025

Copy link
Copy Markdown
Collaborator Author

Alright, let me go ahead and merge this then. Thanks a lot for your support on this.

@memo33 memo33 merged commit a93c7a5 into NAMTeam:staging Apr 28, 2025
2 checks passed
@memo33 memo33 deleted the redundant-adjacencies-removal branch April 28, 2025 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants