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

feat(Dbc/Db): moved dbc tables to separate database #21091

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Razor2142
Copy link

Changes Proposed:

A proposal to move the dbc override tables from the world database to separate one.

This PR proposes changes to:

  • Core (units, players, creatures, game systems).
  • Scripts (bosses, spell scripts, creature scripts).
  • Database (SAI, creatures, etc).

Issues Addressed:

  • Closes

SOURCE:

The changes have been validated through:

  • Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
  • Sniffs (remember to share them with the open source community!)
  • Video evidence, knowledge databases or other public sources (e.g forums, Wowhead, etc.)
  • The changes promoted by this pull request come partially or entirely from another project (cherry-pick). Cherry-picks must be committed using the proper --author tag in order to be accepted, thus crediting the original authors, unless otherwise unable to be found

Tests Performed:

This PR has been:

  • Tested in-game by the author.
  • Tested in-game by other community members/someone else other than the author/has been live on production servers.
  • This pull request requires further testing and may have edge cases to be tested.

How to Test the Changes:

  • This pull request can be tested by following the reproduction steps provided in the linked issue
  • This pull request requires further testing. Provide steps to test your changes. If it requires any specific setup e.g multiple players please specify it as well.

Known Issues and TODO List:

  • Consider removing the _dbc suffix from the table names. This requires changes to this logic.
  • Add migration logic between databases to avoid possible data loss on existing databases. I would have prefered to move tables but there is no way to know the name of the databases from the sql update files.

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

@github-actions github-actions bot added DB related to the SQL database CORE Related to the core file-cpp Used to trigger the matrix build labels Jan 4, 2025
CREATE TABLE `achievement_category_dbc` (
`ID` int NOT NULL DEFAULT '0',
`Parent` int NOT NULL DEFAULT '0',
`Name_Lang_enUS` varchar(100) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Small thing, and likely out of scope to be fixed by this PR, but just a reminder that these locale columns are incorrectly named and do not reflect the actual locale order as seen in client distributions, can be seen in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I would prefer to consider this as separate PR. But from looking into it I found an issue with the Keira. I will look into it and make a PR there.

@heyitsbench
Copy link
Contributor

Of note is that AreaTrigger.dbc is not currently read by AC afaik, instead using the table areatrigger which just seems to mirror the data in that DBC iirc (more info can be found with this PR). At some point, the handling of this data should be adjusted to match the other DBC files.

@Nyeriah
Copy link
Member

Nyeriah commented Jan 5, 2025

I'm not sure spell_dbc should be moved elsewhere, it's not a "replacement" of the dbc file, but rather the storage of serverside spells

@heyitsbench
Copy link
Contributor

I'm not sure spell_dbc should be moved elsewhere, it's not a "replacement" of the dbc file, but rather the storage of serverside spells

IMO I think serverside spells should be split off into their own table, left in world DB, with spell_dbc being moved into this proposed separate DBC DB.

@sudlud
Copy link
Member

sudlud commented Jan 5, 2025

What I'm concerned about is the migration procedure for existing servers.

  • You would have to manually create a new database (most likely requiring to still know the root password of your MySQL installation)
  • also correctly assign the rights for this database to the existing database user
  • And then also correctly update the server configs with the new setting

Something like this? Or am I making wrong assumptions here

@heyitsbench
Copy link
Contributor

What I'm concerned about is the migration procedure for existing servers.

Given that the various DBC tables are meant only to act as overrides to existing DBC files, I'm curious if a connection to a DBC DB could be made optional.

@sudlud
Copy link
Member

sudlud commented Jan 5, 2025

Some thoughts on this

  • keep server side spells in the table spell_dbc in world db, move the rest of the dbc tables to new db
  • only drop empty dbc tables in this pr to not delete custom
    Content automatically
  • make dbc overwrites completely optional feature
  • set feature default off with this pr
  • provide a guide for people using the feature in how to migrate, enable, configure it

The impact on the normal user must be as minimal as possible imo and should never require any manual steps from them to continue using AC after an update.
Also we should never surprise the user with a drop table if there is custom content in their db imo.

@sudlud
Copy link
Member

sudlud commented Jan 5, 2025

Before this should continue any further, can you please elaborate on the advantages this pr would bring?

If it's just "having less to scroll through in the database viewer" then the impact on the normal user would probably not be justifying this change.

@Razor2142
Copy link
Author

I see this as a data segregation issue. While the world data is exclusive to the server and might be modified during runtime the dbc data is shared with the client and read-only.

Although "having less to scroll through" or as I say getting better overview is an important aspect for devs, as not everything can be done by just using Keira. Just viewing this from a normal user point of view might suggest to use a single database for everything as it would be easier.

I agree on the migration issue and will look into it if the PR will be considered for a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CORE Related to the core DB related to the SQL database file-cpp Used to trigger the matrix build Ready to be Reviewed Waiting to be Tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants