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

Progress on Custom Character Creation #568

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

GuyAnyway
Copy link

Enable the custom character creation menu, implement the attributes/abilities menu, provide stubs for skills and feats menu.

Following shortcomings exist:

  • description of the attributes is not displayed on hovering
  • recommended seems to load all-zeros
  • attributes are not yet written to the charcterinfo
  • no check/prompt that points remain

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

  • this is my first C project (have some experience in C)
  • currently have no way of running the original engine (no windows box, wine seems to not work)
  • my only experience with KotOR (I,II) is from decades ago as a plain user

@DrMcCoy
Copy link
Member

DrMcCoy commented Jan 13, 2021

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:

  • The GUI methods that find a specific widget (like getButton()) can return nullptr, if the widget doesn't exist. If you haven't made sure the widget does exist, you should probably put the result in a local variable and couch the deref in an if-block. This can happen, for example, when playing the Xbox version of the game, which has some GUI differences. We could use a less cumbersome way there, btw
  • Commit af5adeb does a whole lot. Can you perhabs split it into several commits?
  • Ability points for point-buy and other values are maybe found in 2DA files. At least that's what the chargen in NWN does

currently have no way of running the original engine (no windows box, wine seems to not work)

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.

@GuyAnyway
Copy link
Author

GuyAnyway commented Jan 13, 2021

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:

  • I was not intentionally aligning code - have to find those instances again.
  • I was surprised that you did not point out the #defines and missing Doxygen comments. ;)
    I plan to provide the latter as well and look into properly replacing the former.

Other:

  • I was aware of getButton() and the like could technically return NULL, I wondered why I should and others did assume that it actually could happen. Now I know. Thanks. :)
  • I will look into splitting the huge commit. (Seems to be different from project to project what size people are comfortable with.)
  • I am generally aware that values for point-buys may be somewhere in the 2DA files. Just have to find out, where.
  • Tbh, I didn't use the wiki a lot. I mostly found my way around by looking at code and the game files.
  • Currently the recommendation button does not work. Because the values for the class-specific abilities are read from the 2DAfiles while creating random characters in the class selection, I thought I just try to read those abilities for the recommendation from the character info. But this returns all-zero. Is this a known issue?
    Also this way there is an issue when saving the abilities but later coming back and pushing the button for recommendations. So the recommended abilities should be rather read on button press I guess.
  • I was not able to implement the display of class descriptions, because I did not figure out how to use the ListBox correctly. How do I find out how to do this?
  • Currently there is no way to write the chosen abilities to the character info (the class doesn't provide a method). Should I implement such a method? If so: As part of this PR or rather a different one?

@GuyAnyway
Copy link
Author

This was just the splitting of the huge commit into three smaller ones.
I hope these portion sizes are more welcome?

The most important remaining question is:

  • I would assume that if this menu exists, it would be quite useless if any of those buttons was missing. Also checking for all those buttons would make the code quite bulky.
    I would like to think that I can leave those unchecked.
    Would you agree or would you rather have the buttons checked?

I plan to still do the following:

  • Add Doxygen documentation
  • Try to locate the values for point-buys in the 2DA files
  • Change the mechanics of the recommendations button: On click read from the 2DA files
  • Save the selected ability points to the character info
  • Figure out how to display ability descriptions/use the ListBox

Do you have a strong opinion which of the above should be part of the pull request?

@DrMcCoy
Copy link
Member

DrMcCoy commented Jan 16, 2021

I hope these portion sizes are more welcome?

Yes, that looks good, thanks! :)

I would assume that if this menu exists, it would be quite useless if any of those buttons was missing

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 :)

Do you have a strong opinion which of the above should be part of the pull request?

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

@GuyAnyway
Copy link
Author

One thing I noticed, I get a segfault when clicking the "Recommended" button during the Ability point allocations.

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 hope to address this when reading those values directly from the 2DA files.

@GuyAnyway
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants