Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use UUID for quest ID, and split up giant quest JSON file #89

Merged
merged 53 commits into from Mar 19, 2023

Conversation

D-Cysteine
Copy link

@D-Cysteine D-Cysteine commented Feb 18, 2023

This PR contains the following changes:

  • Quests now use randomly-generated UUIDs, rather than consecutive integer IDs. This should allow safely deleting old quests, as well as preventing merge conflicts when multiple people try to add quests simultaneously.
    • Well, there is still a 2^(-128) probability of collision
  • Quest lines also now use randomly-generated UUIDs
  • Added a button to copy quest ID to clipboard, since typing a UUID by hand is not fun. Right-click on a quest in the questbook to access this.
  • Quest JSON file has been split up. The new structure is as follows:
    • Default quest database is now saved under DefaultQuests/ directory
    • Quest settings and format version saved at DefaultQuests/Settings.json
    • Quest lines saved at DefaultQuests/QuestLines.json
    • Quests belonging to exactly one quest line saved at DefaultQuests/Quests/<quest line name>-<quest line UUID>/<quest name>-<quest UUID>.json
    • Quests belonging to multiple quest lines saved at DefaultQuests/Quests/MultipleQuestLine/<quest name>-<quest UUID>.json
    • Quests belonging to zero quest lines saved at DefaultQuests/Quests/NoQuestLine/<quest name>-<quest UUID>.json
  • The old save format is available with the command /bq_admin default savelegacy, if needed
  • Quest book translation has been redone; see bottom section for more details

You can see the new format here (little bit stale, and still using integer quest line ID):


Given the large amount of changes needed, particularly the changeover to UUID, I'm sure that there are bugs in here. This PR needs extensive testing; any help would be appreciated! I will leave this as a draft for the time being, until more testing has been done. Here's some pre-built jars:

These are the bugs that I feel are most likely to occur:

  • Mistakes in reading / writing data to NBT. This would apply to saving / loading to disk, as well as client-server communication.
  • Accidentally using == to compare UUID, rather than .equals() (leftover from the old integer IDs, which used ==)
  • NullPointerException from failing to handle null quest ID (leftover from the old integer IDs, which could never be null)
  • ConcurrentModificationException from replacing arrays with collections, and then trying to modify them during iteration
  • Accidentally confusing a quest UUID with a quest line UUID, or a player UUID

I'm sorry to whoever has to review this. I tried breaking the changes up into several commits to make your life easier, but there are still an enormous amount of changes.

Oh, I also deleted a bunch of old code that it didn't look like we were using. This is code for importing from other questbook formats, etc. which didn't seem like it would be at all useful for GTNH. Check the first two commits to see this.


Migrating from old versions: this should theoretically work seamlessly. If the new format database is present, we'll use it. If the new format database is not found, we'll fall back to the old format. Saving will always be done using the new format. There is only one clean-up task that needs to be done post-migration, which is to remove the old format database after the new one is created, but not doing this just means that the old file will be left around and not actually read.

Any old quests or quest lines in the database that use integer IDs will have those integer IDs converted to a UUID with mostly 0's; e.g., 8 becomes 00000000-0000-0000-0000-000000000008. This conversion allows us to carry over old quest database files and player progress.

I wasn't sure what DefaultQuests-us.json was for. It didn't look like it was being read, so I removed it. But please let me know if we need it.


I also made some changes to how translations work. Hopefully, this will reduce or remove the need for separate translation workflows and tools. Translation will now always be on, so it will no longer be necessary to switch out the quest database in order to enable translations. All that will be needed is to provide a .lang file with the appropriate keys. Additionally, when saving in the new database format, an example en_US.lang file will be generated, which will contain the correct lang keys as well as the English quest text.

Here's an example of what the generated en_US.lang file looks like:

Removing code that we don't need, to reduce the amount of code that needs to be updated
Removing code that we don't need, to reduce the amount of code that needs to be updated
…ading code

We should probably refactor this code out of QuestCommandDefaults at some point
This isn't directly related to my other changes, but somehow something I changed seems to be triggering this latent bug.
@D-Cysteine
Copy link
Author

What is the benefit of allowing custom IDs? As I understand it, the quest ID is something that is supposed to be mostly invisible, and only used internally, so its exact value / form isn't something that we should normally need to worry about.

As for the quest clustering in en_US.lang issue, I am planning on grouping them up by quest line, so if that is the concern re: random quest IDs, I'm hoping that this grouping will be enough to resolve that?

@iouter
Copy link

iouter commented Mar 15, 2023

What is the benefit of allowing custom IDs? As I understand it, the quest ID is something that is supposed to be mostly invisible, and only used internally, so its exact value / form isn't something that we should normally need to worry about.

generally sorted by this id.

As for the quest clustering in en_US.lang issue, I am planning on grouping them up by quest line, so if that is the concern re: random quest IDs, I'm hoping that this grouping will be enough to resolve that?

as long as this can be sorted together, preferably in order.

@D-Cysteine
Copy link
Author

Hmm, how about this? In addition to grouping quests by quest line, they are now sorted by (English) quest name:

@iouter
Copy link

iouter commented Mar 15, 2023

Hmm, how about this? In addition to grouping quests by quest line, they are now sorted by (English) quest name:

image
not good

@boubou19
Copy link
Member

i recommend using a string instead of uuid keep related quests adjacent when translating

why translating has anything to do with quest id? The text of the quest is availiable the same way. I can understand that for some questlines, you want to do them one by one to keep text coherent, but it shouldn't matter as translating a quest doesn't need the context of the other quests.

UUID is the ultimate solution for us when we do quest work, we can't ditch it.

@D-Cysteine
Copy link
Author

D-Cysteine commented Mar 15, 2023

Alright, I modified the ordering of quests to do a depth-first traversal of the requirements graph. How's this?

There may be some strangeness in ordering for quests that have multiple requirements, since the logic requires it to be ordered after all of its requirements.

Also worth mentioning, with the new changes, it's possible to do translations in-game, too. Just modify the quest text directly, and then do /bq_admin default save translated; the exported en_US.lang will contain the new translations. There is some manual text file surgery needed, but diff against the prior copy of en_US.lang should give the modified lines, and then those can be copied into the translation file.

@iouter
Copy link

iouter commented Mar 15, 2023

Alright, I modified the ordering of quests to do a depth-first traversal of the requirements graph. How's this?

this couldn't be better, thank you.

@D-Cysteine D-Cysteine force-pushed the improved-database branch 3 times, most recently from 31b4614 to 00d1096 Compare March 15, 2023 17:33
Quests are now grouped by quest line, and ordered within a quest line by an algorithm approximating depth-first traversal of the requirements graph
Copy link
Member

@eigenraven eigenraven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this for dev and sort out any more problems with smaller PRs

@Dream-Master Dream-Master merged commit 2769b7c into master Mar 19, 2023
2 checks passed
@Dream-Master Dream-Master deleted the improved-database branch March 19, 2023 09:38
thefridgeman added a commit to SymmetricDevs/BetterQuesting that referenced this pull request Apr 30, 2023
This is *heavily* based on the following pull request: GTNewHorizons#89

Co-authored-by: D-Cysteine <54219287+D-Cysteine@users.noreply.github.com>
thefridgeman added a commit to SymmetricDevs/BetterQuesting that referenced this pull request May 1, 2023
This commit fixes the compilation errors left by pass 1.
thefridgeman added a commit to SymmetricDevs/BetterQuesting that referenced this pull request May 1, 2023
This commit fixes the compilation errors left by pass 1.
thefridgeman added a commit to SymmetricDevs/BetterQuesting that referenced this pull request May 1, 2023
Automated code cleanup (reviewed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants