refactor(JortPob/Layout): break constructor into separate methods#104
refactor(JortPob/Layout): break constructor into separate methods#104confusingstraw wants to merge 6 commits into
Conversation
| { | ||
| if (content is CharacterContent cc) | ||
| string contentId = content.id.ToLower().Trim(); | ||
| if (allReferences.Contains(contentId) || content is CharacterContent || content is ItemContent || content is DoorContent || content.papyrus != null) |
There was a problem hiding this comment.
Checking against this many types is a bit confusing, especially since the contained switch/if checks are checking other child types. Is there any way to simplify this?
There was a problem hiding this comment.
agree, but this PR is just me moving code into methods. aside from making sure i didn't miss any parameters, i wanted to leave the code as close to identical as possible.
| if (npc == other) { continue; } // dont' self succ | ||
|
|
||
| // guards get bonus range because I said so | ||
| if (other.IsGuard() && System.Numerics.Vector3.Distance(npc.position, other.position) < 50) { npc.witness = CharacterContent.Witness.Guard; break; } |
There was a problem hiding this comment.
It'd be good to put the magic numbers in this method into static vars. Perhaps citizenAlarmThreshold, guardChainingDistance, citizenChainingDistance.
|
|
||
| foreach (Tile tile in tiles) { CheckWitnesses(tile.npcs); } | ||
| foreach (InteriorGroup group in interiors) { | ||
| foreach (InteriorGroup.Chunk chunk in group.chunks) { CheckWitnesses(chunk.npcs); } |
There was a problem hiding this comment.
Is it possible that the overlap of chunks could lead to nearby npcs placed in different chunks missing some of the witness chaining logic?
|
|
||
| /* Render an ASCII image of the tiles for verification! */ | ||
| Lort.Log("Drawing ASCII art of worldspace map...", Lort.Type.Debug); | ||
| private void RenderWorldspaceAscii() |
There was a problem hiding this comment.
Should change this to a stringbuilder.
|
|
||
| // see if map point has already been made. some areas have multiple entrances or exits | ||
| var lowerName = name.ToLower().Trim(); | ||
| if(pointNames.Contains(lowerName)) { continue; } |
There was a problem hiding this comment.
Should move this if above the previous if, but leave adding to the pointNames where it is. It's a lot cheaper to check if a collection contains a string than if compare vector distances on an n-sized collection.
Co-authored-by: Bret D. <horfius@users.noreply.github.com>
Co-authored-by: Bret D. <horfius@users.noreply.github.com>
Co-authored-by: Bret D. <horfius@users.noreply.github.com>
Co-authored-by: Bret D. <horfius@users.noreply.github.com>
2 participants
This PR refactors the
Layoutconstructor so it isn't just a giant monolith. The code remains unchanged, it's just been broken up into separate methods to improve readability and make it easier to get a foothold when making changes.