Add the ability to lock states, provinces, cultures, and religions

[Minivera]

Description

This PR adds the ability to lock states, provinces, cultures, and religions using pretty much the same UX as locking burgs. They all work by making sure the cells keep their own for each respective types and by preventing whatever is regenerated from taking over the cells of locked elements. Some of the code is a bit messy since I started with cultures, then moved to religion (the most arduous one), and finished with states/provinces (the easier ones, which alloweb me to think about some optimizations). I've made sure to document the code along the way so it makes sense.

I've recorded some videos of the feature working.

Cultures

Here are the culture regeneration working with a locked culture. The colors can overlap one another since there isn't any logic for that when a culture is regenerated, but it is properly kept as-is. I've also made sure to prevent cultures from using one of the locked culture cell as their origin.

Enregistrement.d.ecran.le.2022-12-07.a.21.00.09.mov

Religions

The most complicated one and possibly the one most prone to issues. I've added the same features as for the cultures, but I've also made sure that any locked religion will be replaced alongside the other religions in the religion origin tree. At the moment, the tree doesn't keep the old relations alive if all religions is a branch are locked, something for later.

Enregistrement.d.ecran.le.2022-12-09.a.16.49.51.mov

States

States will be locked and no state may use its cells as its capital. It also locks all provinces in a locked state, the assumption is that a locked state shouldn't change at all. Since province generation is part of a state's generation, that locks itself as well.

Enregistrement.d.ecran.le.2022-12-09.a.16.50.38.mov

Provinces

Was mostly built as part of the state locking. It will regenerate all provinces except the locked one (either from state or province) and the province generation makes sure that cells can't be stolen by newly generated provinces when in the same state (can't happen between states).

Enregistrement.d.ecran.le.2022-12-09.a.16.52.02.mov

The bug at the end of the video where I have to click to open the editor again to refresh the list has been since fixed.

Type of change

• Bug fix
• New feature
• Refactoring / style
• Documentation update / chore
• Other (please describe)

Versioning

• Version is updated
• Changed files hash is updated

[netlify]

✅ Deploy Preview for afmg ready!

Name	Link
🔨 Latest commit	a1621d5
🔍 Latest deploy log	https://app.netlify.com/sites/afmg/deploys/639b367142b7f50008a8954d
😎 Deploy Preview	https://deploy-preview-902--afmg.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[Azgaar]

Hello and thanks for the PR. I will try to change it soon. It may need some changes to inline with other code and make it easier to migrate to a new codebase (see #842)

[Minivera]

I've noticed a few bugs while testing on my own map in the preview:

1. The state's name still changes when its locked and we regenerate the states, it should probably not do that.
2. New provinces still seem to be able to generate in a locked state, which breaks the point of locking them during the state generation. (Seems to be because the state can still be normalized and expand sligthly even when locked).
3. A locked state will still block province generation when clicking on the button to regenerate provinces. We should maybe not do that?

I'll be fixing those soon.

[Azgaar]

Awaiting for your confirmation and then will review. There are some concerns on performance and also whether it will be possible to move the code to the vite branch as well (please check it if you can).

[Minivera]

whether it will be possible to move the code to the vite branch as well (please check it if you can).

I've done a quick check and besides code moving from one file to another because of the vite migration, I think the main thing that could break would be the removal of state.provinces. I'll make sure I'm not using it so it can easily be removed. Do you see anything in that PR, given your knowledge of the vite PR, that could break it? Happy to do a follow up commit on the vite PR if it can help.

[Azgaar]

It's not directly related to the PR, but there is an undecided question on whether we need to split the code for the 1st generation and regenerations. 1st generation usually requires less attributes and has to be very fast. Regeneration is a result of user's action, it can take more time, but usually it's more complicated. Like in our case 1st generation doesn't need to consider locked elements, also for the 1st generation we are sure about the data state.

I am not yet sure whether it makes sense to split the function into 2 ones or keep everything in one place. Generally the code should be as simple as possible, so splitting may have sense. But it will require some rework.

[Minivera]

I've fixed most of the bugs I found, but I may have missed some. Let me know if you find anything!

For bug 3., basically the problem is that, if I lock a state, but not the provinces of that state, the province regeneration will still ignore the provinces. I wonder, if I regenerate the provinces, should the state lock still prevent regeneration? Or should it only consider the state lock if regenerating states (which regenerate provinces too). What do you think?

[Minivera]

I am not yet sure whether it makes sense to split the function into 2 ones or keep everything in one place. Generally the code should be as simple as possible, so splitting may have sense. But it will require some rework.

In my honest opinion having worked in the codebase for a few days, I do think some separation should happen. One of the toughest part of the feature (especially for religions) was trying to figure out where to handle locking in regeneration given the code includes both. There was a bug, for example, where the labels would regenerate for lock states and my fix broke when loading an existing map, I had a hard time finding where I need to fix that. I think that's a bit out of scope for this PR though. Maybe a good opportunity for a follow up?

[Azgaar]

For bug 3., basically the problem is that, if I lock a state, but not the provinces of that state, the province regeneration will still ignore the provinces. I wonder, if I regenerate the provinces, should the state lock still prevent regeneration? Or should it only consider the state lock if regenerating states (which regenerate provinces too). What do you think?

Both solutions work fine, I don't have a preference here.

[Minivera]

Both solutions work fine, I don't have a preference here.

I went with the second option using an added parameter to the generateProvince function, I think it creates a better UX than provinces no regenerating without explanation.

[Minivera]

@Azgaar I fixed all the bug I found while testing with random maps and my own maps, I even saved my personal campaign map and it's loading and regenerating fine with this version. I think it's ready to review.

[Azgaar]

@Azgaar I fixed all teh bug I found while testing with random maps and my own maps, I even saved my personal campaign map and it's loading and regenerating fine with this version. I think it's ready to review.

Good, I will try to review today. I also see some potential improvements. Not like it all was good before, the code is very messy, but it will make sense to improve and we add new features. Like all updateLockStatus functions are very similar, so we can generalize them and make shared.

[Minivera]

Good, I will try to review today. I also see some potential improvements.

For sure, please let me know if you'd like me to implement some here or as a follow up 🙂. I also have a few improvements in mind that I was thinking about suggesting in discussions. One big thing I'd like to do, for example, is to break up the generation files between domains. Like instead of a religion-generator.js file, it might be easier to work in the codebase if we broke it up in files in a religion like generate.js, contants.js and so on. Though I think at this point the Vite PR might be a must 🤔

[Azgaar]

Yes, some enhancements can be made later. Big redesign goes as this Vite migration/refactoring.

[Azgaar]

I will merge to the PR to a preliminary branch, then make a few changed and marge to master if everything is fine

[Minivera]

No problem, let me know if I can help with anything!

[Azgaar]

No problem, let me know if I can help with anything!

Vite branch can take years to come, so if you want a see the result in production faster, you can take some current buggy code like Submap. It's an important functionality, but it's not working in lots of cases. Also see our trello board and issues list here. It may be better to concentrate on bugs.

[Azgaar]

Started to work on refactoring, I see a lot of places where code is not effective and can be enhanced. Like when we regenerate states old id should gone, we should not store it and use for some operations, it will create a mess (also it is a data model change). The refactoring progresses slowly, so it may take some time.

[Azgaar]

Fully merged now.