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

Various Spellstrike issues #14

Open
13 of 16 tasks
xyzzy42 opened this issue May 8, 2024 · 10 comments
Open
13 of 16 tasks

Various Spellstrike issues #14

xyzzy42 opened this issue May 8, 2024 · 10 comments

Comments

@xyzzy42
Copy link
Contributor

xyzzy42 commented May 8, 2024

I noticed these while working on the macro.

  • Standby spell allows picking a cantrip slot to expend, which then causes an error.
  • Standby spell could filter the list of spell slots to only those which are allowed to expend, rather than present the whole list and then reject the choice after it's made.
  • If the attack misses, there's no way to know the strike was a spellstrike and what spell was used from the chat message. Maybe the spell should added to the strike roll text somewhere.
  • If hero point reroll is selected, all the other choices in the dialog are ignored. This is not obvious from the UI. Reroll should disable all the other elements or hide that section of the dialog. It'd be even nicer if you could trigger it from the chat card like normal strike reroll.
  • If standby spell is selected, then the hero point reroll option appears, but selecting it has no effect. It will just do a normal spellstrike without using a hero point. There is no indication in the UI that this will happen.
  • Having to select standby spell with another dialog before the main one is slow. It also doesn't tell you what the spell is or what slots you have. It'd be nicer if using standby spell was on the main dialog as a checkbox, so the spell and slots could be seen and the checkbox would only need a click if it was going to be used.
  • Should use the foundry Dialog.confirm() factory method instead of making Yes/No dialogs.
  • The way the spell data flags are injected into the attack roll, by editing the most recent chat message, is subject to races with another chat message appearing while the roll is being made. The message is supplied to callback so it's easy to get the right one without having to search for it.
  • If all spells are considered isExpended or isUseless, the no spells warning isn't shown but there are no spells to choose from. Should probably just filter these out of the spell list. Or skip spontaneous groups with no uses left when making the list.
  • The code to skip ritual and item spellcasting entries doesn't work due to incorrect logic. But they are gone from the system now.
  • There's piles of code to create flavor text for to certain spells, such as Ray of Frost or Gouging Claw, but the text is never added to any messages. Only spells which construct a custom damage roll will use it.
  • If a spell has additional effects, like a condition given to the target, it doesn't appear anywhere. Maybe the spell description normally shown when the spell is cast could be added to chat somewhere.
  • The system adds persistent damage to damage rolls now and it doesn't need to be an extra effect.
  • speaker and flags are not valid options to toMessage()
  • Code to sort spell variants compares lvl but there is no field of that name, it's castRank.
  • Stick the cue marker used on the damage/critical buttons on attacks to indicate which button to use based on the DoS onto the spell damage button to pick, rather than the notification message.
@Symon-S
Copy link
Owner

Symon-S commented May 9, 2024

This macro was written quite some time ago.
It is also kept simple to be easily maintainable as the system changes quite aggressively and quite frequently, so the less components to it, the less potential failure points.

Looks like standby spell filtering broke at some point and needs to be fixed, so I will look into it.
Standy Spell is actually changed and set by another macro, which is how you, as the player, would know what your standby spell is. I guess I could have it rename the spell when set, but I haven't received any feedback on it for the last 2 years, so not really sure how necessary it would be.

For the whole limiting spells based on selections, listeners would need to be setup to change the contents of the selection menus. I was at one point going to go away from the prefab Dialogs I have been using since like v0.8 of foundry, and get to making those, but since they weren't broken, it was put on the backburner.

The reroll using hero points could be optimised, but it can't be triggered by the performed roll, since that may mean multiple duplicate hooks or a hook setting/disabling another hook on every use, this also means if a refresh is needed for one or the other reason, that the hook would be lost. This is more module territory than macro territory. (No I don't contribute or work on worbench.)

As for posting the chosen spell in the strike, that could be easily done, and I will look into it.

It possibly needs a bit of a, or almost complete, rewrite, but since I don't personally use it anymore, I haven't been actively working on it.

I will take these suggestions into consideration as some of these affect the other two macros which this one originally comes from.

Most macros are written to serve a purpose through simplicity as opposed to modules that usually have nicer interfaces and are more actively developed. Which is why some of the macros, especially the contributed ones, have not been updated or worked on in quite some time.

If you would like to integrate spellstrike into a module, or contribute code to it, you are more than welcome to as well.

@Symon-S Symon-S closed this as completed May 9, 2024
@xyzzy42
Copy link
Contributor Author

xyzzy42 commented May 9, 2024

I wanted to make a note of the things I noticed. I've fixed some since then.

I think reroll from the card could be done via an embedded macro link in the chat card html. Could even the spellstrike macro itself, not yet another macro. The macro can find the context it was invoked from and determine it was as a reroll from a strike card. Pretty sure I can do that without issues. Is that actually better? IDK, maybe not.

@Symon-S
Copy link
Owner

Symon-S commented May 9, 2024

I fixed several other things as well, such as filtering out cantrips when standby spell is chosen, switched to a Dialog.prompt for standby spell, center aligned the text when a stanby spell is going to be used, injected the spell link into the strike.

I guess it could just be placed in the ChaMessage with it's applicable variables 🤔

@xyzzy42
Copy link
Contributor Author

xyzzy42 commented May 9, 2024

The strike chat message already has everything needed to reroll. The macro gets the event that triggered it, that has the html element of the button, which can be walked up to some top level chat card div if it was invoked from a link in a message, and that element has data with the message ID. The message has the flag world.macro.spellCast with all the spell data. It would be pretty easy really, maybe even less code with all the UI stuff that could go away.

But there would be this "reroll using hero point" link in every spellstrike message. Don't really like that. Right click menu would be the way, but I can't see how to do that.

My idea for standby spell would be to get rid of the initial dialog for it and put it on the main one. Have a line:

Use Standby Spell: ☑ Shocking Grasp Rank 2

When selected, it does the "Choose a Spell" / "Choose a slot to expend" text change and filters the spell list to disable or hide the incorrect options. Or undoes it when unselected.

@Symon-S
Copy link
Owner

Symon-S commented May 9, 2024

Could just add a button with a listener instead
image

But I will see what I can find 🤷‍♂️

@xyzzy42
Copy link
Contributor Author

xyzzy42 commented May 9, 2024

How do you add a button with an listening function that doesn't go away when the client reloads? Embedded all the javascript into the html of the flavor text?

@Symon-S
Copy link
Owner

Symon-S commented May 9, 2024

I was hoping to just have to embed the call to execute the script 🤔
This would be way easier with a module obviously, since it could just fire off on an update to that message.
I was just going to have it throw in a bypass argument when executing it from the button.
Nevermind, it won't work that way.

@xyzzy42
Copy link
Contributor Author

xyzzy42 commented May 29, 2024

Updated issue list. Lots are fixed now. Found some new ones.

@Symon-S
Copy link
Owner

Symon-S commented Jun 2, 2024

There's piles of code to create flavor text for to certain spells, such as Ray of Frost or Gouging Claw, but the text is never added to any messages. Only spells which construct a custom damage roll will use it.

These were originally added to have the damage card add certain things, like persistent damage bleed/fire/etc or additional damage when hitting a creature with a specific trait. Or in the case of Acid Splash, add splash, and add persistent damage on a critt. With changes in both the system and the macro, some of these have become obsolete. But with others, they will have to be injected somehow into the rolls as this is a feature as opposed to a detriment. I believe these spells are very few now.

If a spell has additional effects, like a condition given to the target, it doesn't appear anywhere. Maybe the spell description normally shown when the spell is cast could be added to chat somewhere.

Since a link is added to the spell description in the strike, additional effects that are draggable, such as conditions could be easily dragged from the pop up. Throwing a spell description for attack spells is another way to clutter the chat, which is already pretty cluttered when rolled.

There may be a better way of doing this. The descriptions could be posted as tooltips instead of to the chat itself.
Hover over the linked spell and see if it has something needing to be dragged and dropped. Maybe just post any Links within the description to the flavor of the damage rolls. Either way minimizing chatlog debt is one of the reasons this macro even exists.

@Symon-S Symon-S reopened this Jun 2, 2024
@xyzzy42
Copy link
Contributor Author

xyzzy42 commented Jun 2, 2024

Yes, I think the flavor text should be added to spells with damage rolls too, not just those without. It would probably look different than what's generated (and not used), since there is already a damage card to put it into, but extra information could be added. The spell effects that aren't shown are something that could be here too.

And some spells filling the entire chat log with a description every time they are cast is indeed very annoying. But just the effect link, if the spellstrike outcome produces one, would be nice.

I looked at removing the stuff for persistent, splash, etc. that the system does now. I think there weren't that many that were entirely obsolete because often something other than double damage happens on a crit and the system doesn't handle any special crit damage.

Speaking of minimizing chat log, it's possible to combine the damage roll from both the strike and the spell into a single damage card. Not a single damage instance with damage combined! But like base + splash have their own buttons in one card and can be applied separately. I'm not sure if that's better, as the all the flavor text, traits, the attack title, and also the data in the Message object about the item doing the damage, would be an issue to share with both damages in one card.

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

No branches or pull requests

2 participants