-
Notifications
You must be signed in to change notification settings - Fork 121
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
Progress on Custom Character Creation #568
base: master
Are you sure you want to change the base?
Conversation
Nice, thanks! I'll have a proper look over it tomorrow or Thursday. :) (Ignore the failed AppVeyor build, that's currently broken. I haven't been able to fix it yet.) A few things:
Interesting. Wine should work. I've got it working here. If you haven't gotten it to work by then, I could maybe see if I can recreate a clean WINEPREFIX and get the Steam or GOG version to run in it. KotOR2 does have an official Linux port now, btw, courtesy of Aspyr. It's available on Steam (but not GOG, for some reason?). There are a few changes made to it, though, and I'd like to have xoreos support both the original and the Aspyr update. If you haven't already, please also have a look over the Developer Central page on our wiki. It includes some basic things like project structure, code style and commit guidelines. You seem to be following it for the most part, but the commit message for af5adeb isn't quite correct (it should be "Progress", with an uppercase P) and I see a few times where you are using tabs to align, not just for indenting. Nothing major, nothing to lose sleep over, just keep an eye on that. If there's anything unclear or missing on the wiki (or code comments or documentation in general), please let us know and we'll try to fix that. |
Firstly: Thanks for the extensive reply. I'll try to incorporate the mentioned issues. Secondly: I read through coding style and other developer related docs. So:
Other:
|
This was just the splitting of the huge commit into three smaller ones. The most important remaining question is:
I plan to still do the following:
Do you have a strong opinion which of the above should be part of the pull request? |
Yes, that looks good, thanks! :)
Possibly :P. Right now, the Xbox version in xoreos doesn't allow really changing a character anyway, I just hacked it to allow accepting a dummy character on enter. I'd say your fine as is currently :)
Adding Doxygen comments would be very nice to have in this PR. The rest sound like they can happen in follow-up PRs One thing I noticed, I get a segfault when clicking the "Recommended" button during the Ability point allocations. Output of the sanitizers here: https://gist.githubusercontent.com/DrMcCoy/8740e14d7928f9663396d6bf5597dd50/raw/359dc957c41d6b65102e70340b0d3b9785f8b185/gistfile1.txt |
I am aware of this.This happens because the recommended values that are read are all-zero, which is out of the 'possible'/valid range. This is outside the range of checks and doing calculations on this lets them underflow - everything is messed up. |
I provided Doxygen comments in the header files only. Is that how it is supposed to be done in this project or should I add the comments to the source file, too? |
363422c
to
b53b174
Compare
Enable the custom character creation menu, implement the attributes/abilities menu, provide stubs for skills and feats menu.
Following shortcomings exist:
This first contribution is meant to be my first step into the project. I wanted to prove to myself that I can make significant contributions.
Please note that