Graveyard Quality of Life - #11794
Conversation
| } | ||
|
|
||
| // Graveyard burial plots. | ||
| if (building instanceof BuildingGraveyard graveyard) |
There was a problem hiding this comment.
This is always BuildingGRaveyard, no?
There was a problem hiding this comment.
It should be, agreed. I have been getting in this habit to protect against the case where some add-on author puts the module on another building for some reason...
There was a problem hiding this comment.
tbf, the grave positions should probably live inside the module just, then we don't care at all, right?
There was a problem hiding this comment.
That is a good idea, but I wonder if the risk of making that change for an existing colony introduces too much complexity or risk for migrating the data upon the upgrade.
There was a problem hiding this comment.
I think that should be fine. It's long term more stable instead.
There was a problem hiding this comment.
Refactored and retested - including a one-time migration from the building-owned graveyard positions.
| visualGravePositions.add(new Tuple<>(graveLocation, graveFacing)); | ||
| } | ||
| // Safely migrate grave positions from the building to the module | ||
| getModule(GraveyardManagementModule.class).migrateLegacyGravePositions(compound); |
There was a problem hiding this comment.
I think we can simplify the migration logic, we just call from here the module "loadfromnbt" method, and we give it the compound where it can load it from, so it doesn't need a special method
There was a problem hiding this comment.
I don't think we want to call the whole loadfromNBT - that's why I did the targeted migration of just the plot positions, because we're merging the plot positions from the building into the module data that already exists. If you are open to retaining the targeted migration I'd prefer that as safer...
2 participants
Checklist
AI was used for code review.
Related issues
Closes Discord Feedback
Changes proposed in this pull request
Testing
Review please