mod-guildhouse icon indicating copy to clipboard operation
mod-guildhouse copied to clipboard

Make IDs easier to edit

Open dedmen opened this issue 2 years ago • 12 comments

In case you need to edit the Entity/GameObject ID's it can now be done in just 4 places. Instead of in two dozen

  • Closes https://github.com/azerothcore/mod-guildhouse/issues/44

dedmen avatar Nov 02 '23 22:11 dedmen

ERROR: Permission to DedmenWoW/mod-guildhouse.git denied to pangolp.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I tried to make modifications and solve the conflicts, but when it comes to make the push, I find that I don't have permissions to upload the changes. If you could enable those permissions, I would push the changes. @dedmen

pangolp avatar Apr 08 '24 01:04 pangolp

Huh weird the checkbox isn't present to allow access to maintainers. I've sent you invite to my project to give you access. Thank you

I just checked for changes here 3 days ago :D

dedmen avatar Apr 08 '24 10:04 dedmen

Huh weird the checkbox isn't present to allow access to maintainers. I've sent you invite to my project to give you access. Thank you

I just checked for changes here 3 days ago :D

From the mobile phone, when I want to open the invitation, it gives me a 404 error. However, I will try from the PC. It's strange, usually, they don't have to invite me. Simply modifying the permissions in the fork settings is usually enough.

I have to check if, in addition to solving the conflict, I don't have to make one more change. Because I was not sure if when editing a SQL file, a new hash was created and executed again. Yesterday I confirmed that, and I saw that it does. But without knowing it, I created a third file. After I upload the changes, I will have to test the branch and verify that everything is fine.

pangolp avatar Apr 08 '24 10:04 pangolp

The conflict is resolved, but I think some suggestions about codes or new hardcore are missing.

pangolp avatar Apr 08 '24 11:04 pangolp

I think the changes are compatible, but we still need to review the hardcore numbers part.

pangolp avatar Apr 08 '24 11:04 pangolp

The hardcoded numbers will also get worse when we add SQL patches, like we did with the previous PR you merged. The update script again has a hardcoded number in it, which again needs to be changed.

Maybe we could store the "base number" in the database, some table. Then c++ code can fetch it from there, SQL update scripts can fetch the base number from there, and we can make it changeable in one single place (the initial init SQL script)

But that's a quite bigger change. I didn't want to provide a full solution here, I just wanted to make it a bit easier (edit 4 places instead of over 20), and the better solution can be done later (more easily because I already did half of it).

Also the way I did it with the constexpr function, means the value needs to be present at compile-time, so putting it into config file isn't possible that way. But it solved my use case as I compile from source with edited ID's (which you already had to do previously)

dedmen avatar Apr 08 '24 11:04 dedmen

The hardcoded numbers will also get worse when we add SQL patches, like we did with the previous PR you merged. The update script again has a hardcoded number in it, which again needs to be changed.

Maybe we could store the "base number" in the database, some table. Then c++ code can fetch it from there, SQL update scripts can fetch the base number from there, and we can make it changeable in one single place (the initial init SQL script)

But that's a quite bigger change. I didn't want to provide a full solution here, I just wanted to make it a bit easier (edit 4 places instead of over 20), and the better solution can be done later (more easily because I already did half of it).

Also the way I did it with the constexpr function, means the value needs to be present at compile-time, so putting it into config file isn't possible that way. But it solved my use case as I compile from source with edited ID's (which you already had to do previously)

If you can't edit it, from the configuration file, then bye-bye to the solution I wanted to give you... What happens if we remove the constexpr and add the value in the configuration file?

pangolp avatar Apr 08 '24 11:04 pangolp

If you remove constexpr it fails to compile. Because the switch cases need to be compiletime constant values https://github.com/azerothcore/mod-guildhouse/pull/45/files#diff-718379843cd95689dd90c3b83f7107660e3c4bdaa7d3930617e3239dfac24f2eR259-R266

Of course can make that not be a switch and instead some different runtime dispatch. But that would be a bigger rework there too that I didn't want to go down to.

Maybe I'd have some time soon-ish to do just that. But probably would be a couple weeks to get to it.

dedmen avatar Apr 08 '24 12:04 dedmen

If you remove constexpr it fails to compile. Because the switch cases need to be compiletime constant values https://github.com/azerothcore/mod-guildhouse/pull/45/files#diff-718379843cd95689dd90c3b83f7107660e3c4bdaa7d3930617e3239dfac24f2eR259-R266

Of course can make that not be a switch and instead some different runtime dispatch. But that would be a bigger rework there too that I didn't want to go down to.

Maybe I'd have some time soon-ish to do just that. But probably would be a couple weeks to get to it.

I leave you the updated branch then, the module compiles and works, when you have time to check it, welcome.

pangolp avatar Apr 08 '24 16:04 pangolp

What this person did, doesn't it work, or is it complementary to what you want to do: https://github.com/azerothcore/mod-guildhouse/pull/39.

pangolp avatar Apr 08 '24 16:04 pangolp

#39 is completely unrelated to my thing.

dedmen avatar Apr 10 '24 19:04 dedmen

if no objections, I would merge this, it improves the code quality

Helias avatar Aug 15 '24 17:08 Helias