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

Display range for spells in monster description #599

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@Sandman25DCSS
Copy link
Contributor

Sandman25DCSS commented Aug 19, 2017

Please see
https://crawl.develz.org/tavern/viewtopic.php?f=8&t=23901
No colors yet, I am not sure how to do it properly.

image

Display range for spells in monster description
Please see
https://crawl.develz.org/tavern/viewtopic.php?f=8&t=23901
No colors yet, I am not sure how to do it properly.
@Sandman25DCSS

This comment has been minimized.

Copy link
Contributor Author

Sandman25DCSS commented Aug 19, 2017

Oh, it should be 24 instead of 25 to have all rows with same size. Sorry.

@jmbjr

This comment has been minimized.

Copy link
Contributor

jmbjr commented Aug 19, 2017

Will there eventually be a title indicating that range is parentheses?

@Sandman25DCSS

This comment has been minimized.

Copy link
Contributor Author

Sandman25DCSS commented Aug 20, 2017

I am not sure it is needed. We don't have it for Hexes chance.

@rawlins

This comment has been minimized.

Copy link
Member

rawlins commented Aug 21, 2017

I think it will need some kind of labeling -- this would be way too cryptic without it. Percents are much easier to guess without explicit labeling, though that doesn't meant that they shouldn't have one too. I would also suggest trying to get some kind of stylistic consistency between the percents and the ranges, either have both before the name or both after, with the same alignment.

For colors, I think you should be able to use <white>...</white> etc. If those don't work there needs to be another wrapper function, I can check on the details later.

I like this change, though I'm more pro-numbers than some so I'm not sure what other devs will make of it. But the fact that LCS range, vs. all the other spells, is so short is a very good example of information that perhaps should be revealed somehow.

@Sandman25DCSS

This comment has been minimized.

Copy link
Contributor Author

Sandman25DCSS commented Aug 21, 2017

Yes, I need some detailed directions what to do here since I am not UI guy, especially for text games with limited space.

@rawlins

This comment has been minimized.

Copy link
Member

rawlins commented Aug 25, 2017

I can work on this directly at some point, but I won't get to it for a little while. I think the simplest thing for labeling the ranges is to provide, before or after the full spell list, some version of the following text: (the maximum range of the spell is in parenthesis). That's what I'd try first, at least. It would also be helpful to provide a console screenshot with an 80x25 term, since that is the limiting case for size.

@Sandman25DCSS

This comment has been minimized.

Copy link
Contributor Author

Sandman25DCSS commented Aug 28, 2017

I believe horizontal size is not changed, I shortened spell name by the same amount of characters as range takes.
Yes, it would be great if someone worked on it, I hope the patch is a good starting place and I really dislike working on UI with all those "let's move that label 3 characters to the left and capitalize it".

Hellmonk added a commit to Hellmonk/hellcrawl that referenced this pull request Feb 27, 2018

Display monster spell ranges (VeryAngryFelid).
Relevant pr: crawl#599. I'll try to add colors or sth soon.
@rawlins

This comment has been minimized.

Copy link
Member

rawlins commented Oct 20, 2018

This is still on our list, and I think @aidanholm might already have a branch that does it.

@aidanholm

This comment has been minimized.

Copy link
Contributor

aidanholm commented Oct 21, 2018

There is indeed: branch pr-599, which IIRC only needs a few spells to have their ranges fixed. It could probably be merged as is, since showing a range of (7) for spells like haste is a fairly minor bug.

@rawlins

This comment has been minimized.

Copy link
Member

rawlins commented Oct 21, 2018

I can take a look at that branch some time in the next couple weeks.

@ebering

This comment has been minimized.

Copy link
Contributor

ebering commented Jan 15, 2019

It seems @rawlins didn't get around to that. The branch needed rebasing to thread in some of the hex chance changes and for some new-ui things (I think; merge conflicts anyway). I've done that work, it is in pr-599-rebase.

Outstanding issues now are:

  • Add the colour and range numbers to WebTiles
  • Fix the issue @aidanholm mentioned with some monster self-targeting spells displaying a range of (7).
@ebering

This comment has been minimized.

Copy link
Contributor

ebering commented Jan 15, 2019

The second checkbox isn't completely fixed. The lone outstanding spell is Invisibility, which players can cast at LOS range (as can monsters), but monster AI will never attempt to cast invisible on anything other than the caster.

Options going forward include

  • Make Invisibility self targeting only
  • Put in a special case for this one spell
  • Ignore the wart for now

I favor the first one.

@ebering

This comment has been minimized.

Copy link
Contributor

ebering commented Jan 15, 2019

Merged, with tweaks and updates to apply cleanly to the current codebase in ab26df7..9fa1570

Sorry this took so long to get done, but as you can see it took a bit of doing in the end.

@ebering ebering closed this Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.