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

gm_notes: allow custom monsters #164

Closed kloetzl closed 6 months ago

kloetzl commented 6 months ago

GM Notes were limited to preexisting monsters.

canismarko commented 6 months ago

Hi, @kloetzl. Thanks for the contribution.

If I understand this right, the monsters will now need to be classes, right? I don't really have a preference which way works. I wonder if the existing system is compatible with what you're trying to do?

Does it work for your use case if you add this to your gm notes file (e.g. gm_notes.py)? I haven't run this code, so I mean it more as an illustration, modify as needed.


from dungeonsheets import Monster

class MyMonster(Monster):
    ...

monsters = [MyMonster(), ...]

It would then be an instance of monsters.Monster and would pass fine.

If that works, then I think this is actually a documentation bug. I will update the docs and examples to make this more clear.

If not, I would propose the following changes to your PR before merging.

1) Include an elif so that we can handle both isinstance(monster, Monster) and issubclass(monster, Monster). Otherwise existing gm_notes break. 2) Add an example of this new usage to examples/gm-session-notes.py

Also, why are you testing that isinstance(monster, type)? Would that automatically be true if issubclass(monster, Monster) is true? Not that I plan this, but adding this check closes the possibility of metaclasses for Monster (maybe we want abstract base classes or something?).

Let me know your thoughts.

kloetzl commented 6 months ago

Apologies, I got myself a bit confused when writing this PR. For magic items, spells, classes, etc. the user always has to provide the class to the array (or at least that is how it's done in the documentation). It is a bit weird that for monsters it has to be an instance of said class. I assumed that this inconsistency was just an oversight.

I've incorporated your suggestions. Feel free to simplify the example.

canismarko commented 6 months ago

I assumed that this inconsistency was just an oversight.

It probably is to be honest. But since it's there, we shouldn't break it.

Looks like there's a missing import in the example, once that's fixed I'm good to merge.

canismarko commented 6 months ago

Looks good, thanks.