-
Notifications
You must be signed in to change notification settings - Fork 108
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
TTS #375
base: master
Are you sure you want to change the base?
TTS #375
Conversation
|
src/brogue/IO.c
Outdated
@@ -3474,7 +3479,7 @@ void flavorMessage(char *msg) { | |||
// arrived on the same turn, they may collapse. Alternately, they may collapse | |||
// if the older message is the latest one in the archive and the new one is not | |||
// semi-colon foldable (such as a combat message.) | |||
void message(const char *msg, enum messageFlags flags) { | |||
void message(const char *msg, enum messageFlags flags, short speechPriority) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not fold speechPriority
into the other messageFlags
? With a judicious default it could probably save touching a lot of message
call sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a great idea, i wanted to avoid modifying the existing enum, but that handles a default much better
@@ -189,7 +189,7 @@ const color fireForeColor = {70, 20, 0, 15, 10, | |||
const color lavaForeColor = {20, 20, 20, 100, 10, 0, 0, true}; | |||
const color brimstoneForeColor = {100, 50, 10, 0, 50, 40, 0, true}; | |||
const color brimstoneBackColor = {18, 12, 9, 0, 0, 5, 0, false}; | |||
|
|||
//Red Green Blue RedRand GreenRand BlueRand Rand Dances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i'm not sure where this came from, probably the IDE doing something.
@@ -252,6 +252,8 @@ int main(int argc, char *argv[]) | |||
continue; | |||
} | |||
|
|||
rogue.trueColorMode = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the existing BrogueSPEAK implementation manually turns all trueColorMode
checks to false
checks, essentially skipping any code that deals with trueColorMode
. i'm not yet positive the reason for it, i'm guessing it's not related to TTS, maybe just a simplification. this should be conditionally false based on whether this is BROGUE_SPEECH
mode, i just removed all of my #ifdef BROGUE_SPEECH
for now because VScode wasn't seeing my define
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does that to simply hard-code colour effects off, to increase clarity for partially sighted players. IIRC there are other small visual improvements to that effect in Brogue-SPEAK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, sounds good to me. if you have suggestions on how to do the checks with the compile flags lmk, my strategy was just like
#ifdef BROGUE_SPEECH
rogue.trueColorMode = false
#endif
or whatever.
VScode wasnt able to handle that properly so i dropped it for now, but if there's a better way i'll do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also going to be busy for the next few days, should be able to work a bit more on this over the weekend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a shame if the TTS features required a recompile (which using #define
would imply.) Maybe we can arrange for it to be {dis,en}abled with a command line flag and/or key binding?
Alternately I'd be interesting in seeing what happens if we just leave true color mode alone; maybe it's fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the TTS features required a recompile
I suppose another way to say this is it would be nice if we could distribute just one binary, and have it serve both users who want TTS, and users who want true color.
I’ll see about toggling TTS just the same as graphics, yeah. TTS needs SDL,
so it would need to have been compiled with graphics, but I believe the
main distribution is?
I’m inclined to trust the author of Brogue-SPEAK on true color, doesn’t
need to be removed in the first draft of TTS but I think it’s probably
something they learned from playtesting with visually impaired people. I’ll
see about getting people to playtest, too
…On Tue, Aug 3, 2021 at 12:40 AM withinwheels ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/platform/main.c
<#375 (comment)>:
> @@ -252,6 +252,8 @@ int main(int argc, char *argv[])
continue;
}
+ rogue.trueColorMode = false;
if the TTS features required a recompile
I suppose another way to say this is it would be nice if we could
distribute just one binary, and have it serve both users who want TTS, and
users who want true color.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#375 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSWLAM2QDVXMMZBQSTU22LT25XLNANCNFSM5BKP4PQA>
.
|
e7ff7ef
to
748f54e
Compare
alright, i have priority/flags folded into |
additionally TTS is toggled at runtime with |
# Enable debugging mode. See top of Rogue.h for features | ||
DEBUG := NO | ||
DEBUG := YES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this line will need to revert dropped before merge.
char locationMessage[DCOLS*3]; | ||
|
||
if(strlen(locationDescription) < 2) { | ||
strcpy(locationMessage, "Blank."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Nothing" or "empty ground" might be more thematic.
|
||
sanitizedMessage[j++]= text[i]; | ||
} | ||
printf(sanitizedMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This printf seems like it will interfere with console interfaces. Is it just debug logging that can be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep! still just figuring out some bugs. occasionally there's some garbage synthesized that i need to figure out
see diff at https://github.com/flend/brogue-1.7.4/compare/feature/brogue174-SPEAKv03?expand=1&w=1
sayIt
s from diffs