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

Draft: /clock command #7

Merged
merged 60 commits into from
Jan 18, 2022
Merged

Draft: /clock command #7

merged 60 commits into from
Jan 18, 2022

Conversation

rsek
Copy link
Contributor

@rsek rsek commented Jan 5, 2022

functional but extremely basic implementation of clocks, as described on p. 230 of Starforged (122421).

currently there's one kind of clock that can be set to one of the standard sizes (4, 6, 8, 10). it's an embed with a single button component that advances the clock by one segment. the Clock class has a constructor overload allowing it to parse a MessageEmbed back to a Clock for manipulation.

things i plan to add:

  • clock image resources for the 32 possible clock states (4 + 6 + 8 + 10 wedges, plus one empty state for each size)
  • campaign clock (p. 231): gets assigned an Ask the Oracle chance.
    • "Advance" button rolls Ask the Oracle. if it's a "yes", it fills a segment. if it's also a match, it fills two segments. otherwise, no segment is filled.
    • user gets adequate feedback so it's clear the AtA roll is happening (and not just a nonfunctional button)
    • dropdown menu component allows changing AtA chance this after embed is generated. alternatively, 1 advance button for each AtA chance.

deferring for later PRs:

  • clocks as embed fields: a compact representation of a clock in a single embed field (+ constructor overload to parse it)
    • combined with a progress track embed to create scene challenge embed (probably with its own slash command)
    • attachable to any progress track
  • campaign clock "dashboard" embed to manage several clocks at once - allow them to be rolled in one go for session start

things i'm still considering:

  • on one hand, the book is pretty specific: "Never erase segments—clocks are inexorable and only move forward. If a clock is no longer a factor due to your actions or because of external events, the clock stops and is removed from play." on the other hand, reversibility is important. could live behind a "..."-type button?

inessential but could be fun:

  • have the embed colour gradate through blue => purple => red as clock fills :D

@rsek
Copy link
Contributor Author

rsek commented Jan 5, 2022

since some clocks leverage Ask the Oracle results (and would ideally report them like any other AtA so that it's clear to users what's happening), i'm also writing a class called OracleAnswer to make AtA more broadly applicable. since they're ultimately just a d% roll, they extend the Die class with several methods and properties, including ToEmbed(), IsYes, and IsMatch.

@rsek rsek mentioned this pull request Jan 5, 2022
@rsek
Copy link
Contributor Author

rsek commented Jan 7, 2022

user-facing aspects of tension clocks and campaign clocks are pretty much done now - please test 'em if you get chance! main thing i could add is an embed so there's user feedback for clock fill events that aren't mediated by Ask the Oracle.

there's definitely some code cleanup i still need to do, though, including some silly workarounds because reflection was giving me grief. but once that's done i think this feature will be fit for human consumption.

since scene challenges have more moving parts (and since they involve progress tracks, which i might want to tweak anyways), i'll defer them for a future PR. they can be simulated with a tension clock + a formidable track in the mean time.

rsek and others added 3 commits January 17, 2022 15:58
commit 0290367
Author: rsek <hello@rsek.ca>
Date:   Thu Jan 6 14:39:56 2022 -0800

    proper files instead of symlinks, oops

commit da988d2
Merge: b4982d8 27082a2
Author: rsek <hello@rsek.ca>
Date:   Wed Jan 5 16:25:48 2022 -0800

    Merge branch 'XenotropicDev:main' into oracle-paths

commit b4982d8
Author: rsek <hello@rsek.ca>
Date:   Tue Jan 4 18:17:11 2022 -0800

    convert Source.Date string to DateOnly

commit 1491258
Author: rsek <hello@rsek.ca>
Date:   Tue Jan 4 18:16:51 2022 -0800

    renamed classes

commit 4dad250
Author: rsek <hello@rsek.ca>
Date:   Tue Jan 4 13:55:26 2022 -0800

    update gitignore

commit 5082808
Author: rsek <hello@rsek.ca>
Date:   Tue Jan 4 13:53:21 2022 -0800

    reorganizing dir, simplifying namespace declare

commit 9bcc389
Merge: 502558d 9a6104c
Author: rsek <hello@rsek.ca>
Date:   Tue Jan 4 14:30:37 2022 -0800

    Merge branch 'XenotropicDev:main' into oracle-paths

commit 502558d
Merge: 017fdca 8e63be0
Author: rsek <hello@rsek.ca>
Date:   Mon Jan 3 21:00:13 2022 -0800

    Merge branch 'XenotropicDev:main' into oracle-paths

commit 017fdca
Author: rsek <hello@rsek.ca>
Date:   Mon Jan 3 20:59:16 2022 -0800

    comments to remove later

commit 1c4e655
Author: rsek <hello@rsek.ca>
Date:   Mon Jan 3 20:54:29 2022 -0800

    deserialize structured oracles

commit b8a3c82
Author: rsek <hello@rsek.ca>
Date:   Mon Jan 3 20:35:40 2022 -0800

    oracle data w/ structured categories in new format

commit f19055b
Author: rsek <hello@rsek.ca>
Date:   Mon Jan 3 18:48:25 2022 -0800

    experimental autocomplete json

commit fb6fbf9
Merge: 43b5cc9 6b2c0d0
Author: rsek <hello@rsek.ca>
Date:   Mon Jan 3 13:59:02 2022 -0800

    Merge branch 'XenotropicDev:main' into oracle-paths

commit 43b5cc9
Author: rsek <hello@rsek.ca>
Date:   Mon Jan 3 02:47:47 2022 -0800

    initial commit
Copy link
Owner

@XenotropicDev XenotropicDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is good to go. There's a few things I wouldn't mind fixing before I merge it in though.

TheOracle2/AskTheOracle/OracleAnswer.cs Outdated Show resolved Hide resolved
return str;
}
}
public static Dictionary<int, string> OddsString => new()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a dictionary<int, string> solely to allow for spaces in the name? Because overall for both code, and possible future localization I think enum with a [DisplayAttribute] is a better way to go, even if it's annoying to get the string value out of the DissplayAttribute

SmallChance = 10
}

public class AskDefs
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't being used as far as I can tell, and I don't really understand the intent of this code. I'm assuming this is something that got left over from some testing or something. I plan on removing it before I merge it in, but let me know if this is for something.

}
public Color OutcomeColor()
{
return IsYes switch
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style only: Overall I'm not a fan of doing this for something that could be handled very cleanly by a ternary operator. If the rest of the code in this class was using switches I'd be way more okay with it. If I'm in this file later on I reserve the right to change it to a ternary, but I'm really not too worried about it.

public EmbedBuilder ToEmbed()
{
string authorString = $"Ask the Oracle: {OddsString[Odds]}";
string footerString = IsMatch ? MatchMessage : "";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the MatchMessage should handle the IsMatch value, and either return null or String.Empty. That way the code is at the point of responsibility.

SelectMenuComponent menu = component as SelectMenuComponent;
var values = interaction.Data.Values as string[];
var results = menu.Options.Where(option => values.Contains<string>(option.Value));
return results as SelectMenuOption[];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think .ToArray() is better here, and it's a little more clear, but not really something to worry about.

return option.Description;
}
/// <summary>
/// Gets the emojis of the originating menu options.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I missing the emoji part of this?

TheOracle2/GameObjects/Die.cs Show resolved Hide resolved
@@ -0,0 +1,59 @@
namespace TheOracle2.GameObjects;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, probably needs to be removed. I'm not sure if this is a rabbit hole we want to start down, but I don't think it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, probably needs to be removed. I'm not sure if this is a rabbit hole we want to start down, but I don't think it is.

gah, i thought i deleted this! reason #534513 not to do such huge PRs, rsek. i made it when i was avoiding working with the database. then i realized that working with the db wasn't nearly so difficult as i assumed.

@@ -0,0 +1,34 @@
namespace TheOracle2.GameObjects;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like where this is going, but I think it should probably use the event pattern, and remove most of the AlterOn stuff. I have some stuff planned for this space, so lets leave this for now, and revisit it in the upcoming player logs PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like where this is going, but I think it should probably use the event pattern, and remove most of the AlterOn stuff. I have some stuff planned for this space, so lets leave this for now, and revisit it in the upcoming player logs PR.

sounds good - it's pretty skeletal and i don't think its method is presently use, but i wanted to reserve some space for it on certain interfaces/classes

@XenotropicDev XenotropicDev self-requested a review January 18, 2022 03:43
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

Successfully merging this pull request may close these issues.

None yet

2 participants