-
Notifications
You must be signed in to change notification settings - Fork 75
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
Vanilla Character Attributes and Skills reset #288
Conversation
Effort to remove legacy attribute functionality, Removes attributes from chrAncestires and chrBloodlines and adds new column baseAttribute in chrAttributes. All attributes are now 20, charisma at 19, to reflect EvE Incursion patch changes
Character attributes are no longer set through bloodlines or ancestries, which is accurate to Crucible functionality
Disabled career skills (not applicable in Crucible) and added migration to change skills to basic starting skills accurate for Crucible. This results in a character around 60k SP
Reviewer's Guide by SourceryThis pull request updates the character attributes and starting skills to match vanilla Crucible values. The changes include refactoring the attribute fetching logic, updating the character creation process, and adding database migration scripts to modify the schema and data. The old methods and logic have been commented out to allow for easy reversion if needed. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @triple111 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
uint32 attributeID; | ||
|
||
if (!res.GetRow(row)) { | ||
codelog(DATABASE__ERROR, "Failed to find attribute information for attribute %d", attributeID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Potential use of uninitialized variable.
The variable attributeID
is used in the error log before it is assigned a value. This could lead to undefined behavior or misleading error messages.
@triple111 Thanks for the contribution! I looked over everything and it all looks good to me and brings us closer to that vanilla experience. Just a couple of things before I merge this:
|
To fix the workflow failure, can you please |
Removed blank lines and unintialzed variable
@jdhirst The requested changes are included in the latest commit. I did however have an error with the workflow failure command you requested:
|
I got this to build so will merge it, the CI pipeline should work for new branches now. |
This PR aligns the character attributes and starting skills with vanilla Crucible values to create a more accurate and vanilla experience. I used historical sources to find the starting values for skills, attributes, and ISK. All changes were non-destructive to allow for reversion. There are 2 new database migrations and changes to several server classes and eve-server.xml.
A new character will now start with:
20 Perception
19 Charisma
20 Intelligence
20 Memory
20 Willpower
~60K SP
5,000 ISK
Reference: Eve University "Starting Skills" of 23 March 2013
https://wiki.eveuniversity.org/index.php?title=Starting_skills&direction=next&oldid=13906
I am investigating the difference between the quoted 56,489 SP value and the resulting value due to these changes.
Summary by Sourcery
This pull request updates character attributes and starting skills to match vanilla Crucible values. It includes new database migrations and modifications to server classes to support these changes. The changes are non-destructive, allowing for easy reversion if needed.