canismarko / dungeon-sheets

A tool to create character sheets and GM session notes for Dungeons and Dragons fifth edition (D&D 5e).
https://dungeon-sheets.readthedocs.io/en/latest/
GNU General Public License v3.0
161 stars 66 forks source link

Several small enhancements #163

Closed PJBrs closed 6 months ago

PJBrs commented 6 months ago

This PR contains four small enhancements:

  1. It makes the MSavage latex template use proficiencies that the user has added in the proficiencies_text variable in their character file. This variable was ignored entirely, and instead the MSavage latex template used an undocumented variable chosen_tools. In order to maintain backward compatibility, the patch only uses proficiencies added in proficiencies_text when none are added in chosen_tools by the user.
  2. It turns the xp variable in the character file to a string instead of an int, so that the user can set Milestone as experience instead of a numer. As far as I can tell, an integer value without quotes is still treated as a string anyway, so this change also should be backward compatible.
  3. As mentioned in one, the user can add proficiencies in the proficiencies_text variable in their character file. However, after a character file has been created with create-character, the character file actually lists _proficiencies_text as the variable, i.e., with an underscore prepended. This is ignored entirely in character.py, causing associated proficiencies to go "missing." Correct the templates so that the user better knows how to add proficiencies such as tools and musical instruments.
  4. Add pdfbookmarks to the fillable pdfs. If I recall correctly, I did this binary change with pdftk. Anyway, it looks nice!
canismarko commented 6 months ago

@PJBrs Thanks for this. Two questions.

Does this change make it required that the XP be a string? I see the desire to put "Milestones" or something in there instead, but XP is fundamentally a number and forcing it to be a string means I can't manipulate it as a number.

For example, my original pattern was to set xp=0 at character creation, then to add the XP for each session, so after 3 sessions it would be xp = 0 + 250 + 175 + 50. That way I have a record of my character XP growth in case I need to go back for some reason.

Also, for

As mentioned in one, the user can add proficiencies in the proficiencies_text variable in their character file. However, after a character file has been created with create-character, the character file actually lists _proficiencies_text as the variable, i.e., with an underscore prepended. This is ignored entirely in character.py, causing associated proficiencies to go "missing." Correct the templates so that the user better knows how to add proficiencies such as tools and musical instruments.

I think that should work. That underscore was from before I added the thing that ignore attributes in the character file that start with an underscore. If you haven't already, would you check a few of the examples to make sure the proficiencies render properly?

Other than that, I think the rest looks good.

PJBrs commented 6 months ago

Well, the xp really turns into a string, and it needs to be turned back into an int to help it keep doing what you want it to do.

One way forward would be to keep xp a string, add int _xp = str(xp) if re.match(r'^[0-9].$', xp),and then use _xp internally for all your calculations. I'll see if I can do that quickly (and let me know if you disagree) and otherwise I'll drop that patch.

With regard to the proficiencies_text thing, all examples except sorcerer_ranger were already using proficiencies_text instead of _proficiencies_text:

$ git grep proficiencies_text examples
examples/artificer1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/barbarian1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/barbarian2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/bard1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/bard2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/bloodhunter1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/cleric1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/cleric2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/druid1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/druid2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/druid3.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/fighter1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/fighter2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/homebrew.py:proficiencies_text = ("dull sword",)  # ex: ("thieves' tools",)
examples/monk1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/monk2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/multiclass1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/multiclass2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/paladin1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/paladin2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/paladin3.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/ranger1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/ranger2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/ranger3.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/rogue1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/rogue2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/sorcerer_ranger.py:_proficiencies_text = ("Cartographer's tools", )  # ex: ("thieves' tools",)
examples/sorceror1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/sorceror2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/warlock1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/warlock2.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/wizard1.py:proficiencies_text = ()  # ex: ("thieves' tools",)
examples/wizard2.py:proficiencies_text = ()  # ex: ("thieves' tools",)

The only thing my patch does, is to remind the user what variable to use. (Although it does strike me that I should have changed the sorcerer_ranger as well; that, and the homebrew one, being the only examples that actually used this variable; the characters I made myself use this much more).

PJBrs commented 6 months ago

On second thought, I missed your point about the XP. Your routine would not work after changing xp from int to string. The alternative would be:

#xp=0
#xp=250
#xp=425
xp=475

If you don't like it, I can alternatively write a patch that assumes the player uses milestone experience as long is xp = 0, or one that sets to Milestone if the player sets it to -1. Let me know what you prefer.

PJBrs commented 6 months ago

... I changed the sorcerer_ranger.py example to use proficiencies_text instead of _proficiencies_text, and it made their Cartographer's Tools show up :-)

Incidentally, something seems to be fishy in that proficiency list for the fillable pdf, compare the fillable one with the latex one (these are both the sorcerer_ranger). Fillable: image

Latex: image

We might want to change to fillable form to using the MSavage latex sheet text... Then again, the MSavage sheet might want to mention something about druids and metal armor...

PJBrs commented 6 months ago

Wait, I have an idea for the XP string int thing!

PJBrs commented 6 months ago

So, I tested some more, and to my surprise it already was possible to just enter "Milestone" for exp, and it just works. And it works for a number as well. It even works for xp = "Milestone " + str(0 + 250 + 175 + 50). So, my patch actually wasn't necessary at all. What remains now is the suggestion in the character file to set "Milestone". I have no idea how python treats this internally, but apparently it's a string when you treat it as a string, and it's an int otherwise.

I did add Milestone to a couple of examples, just to make sure that it sees testing.

PJBrs commented 6 months ago

I double-checked the proficiencies patch on all characters I have made myself, comparing my branch with master. I see no changes in the sense that proficiencies go missing, I only see proficiencies appearing that would otherwise have been lost.

So, @canismarko , is there anything else in this PR that you need me to test?

canismarko commented 6 months ago

I think is good now. Merging.

PJBrs commented 6 months ago

I think is good now. Merging.

@canismarko It doesn't look very merged to me, rather, it looks closed and unmerged...?

canismarko commented 6 months ago

Lol, woops. Should be merged now.