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

Adventure score screen and conditional question polish #81

Merged

Conversation

clpetersonucf
Copy link
Member

@clpetersonucf clpetersonucf commented Mar 20, 2024

  • Adds a custom score screen. Supports all node types and accurately displays inventory item changes and conditional questions (hopefully!)

  • In support of the score screen, the following changes to the score module (and scoring generally) were made:

    • Adjusted the formatting of data sent to the score screen via the get_score_details method.
    • Previously, score logs were matched to the corresponding qset item by a log text comparison, which is prone to inaccuracy. Now, logs of type FINAL_SCORE_FROM_CLIENT will provide the end node's node id as the log's item_id. Older adventure score logs always have an item id of 0 for FINAL_SCORE_FROM_CLIENT log types.
    • Narrative node visits are now included in logs, which was not the case previously. This ensures proper tracking of inventory deltas.
  • Adds partial matches option for Short Answer node types. Updates the short answer node UI to be slightly better formatted.

  • Minor adjustments to conditional question UI. No changes were made to how conditional questions are actually selected.

  • Condensed give/take item and required item dialogs into a single variant, instead of two duplicate UIs depending on layout.

  • Significant rework of the "Add a node in between these two nodes" UI. The in-between button is always visible as a small dot on the midpoint of links where both the source and target are not blank. Additionally, these in-between nodes will shift position when too close to another node to prevent occlusion.

  • A handful of miscellaneous bug fixes and styling tweaks.

Remaining todos:

  • Adjust the advanced question editor button to be positioned adjacent to question text and media options. Did not implement
  • Fix sizing of question text area in the node manager.
  • Handle truncating of excessively long question and answer text in the score screen DNI

Cay Henning and others added 29 commits December 14, 2023 14:29
…on, fix modal toggling, alter button text based on node type, update qset
…em panel to reduce redundancy & fix state bug
Copy link
Contributor

@FrenjaminBanklin FrenjaminBanklin left a comment

Choose a reason for hiding this comment

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

The streamlining and score screen modifications seem to be working properly. Can't get anything to start misbehaving.

@clpetersonucf
Copy link
Member Author

Short Answer updates:

  • Special character sensitivity settings work again.
  • Score screen now displays the actual response text the user submitted, instead of the matched value
  • Updates to partial string detection: The set of potential matches for a particular response is now collected and ranked based on specificity. A match is selected based on the following: exact match (from an answer set that requires exact matches) > exact match (from an answer set that allows partial matches) > partial matches ranked by specificity (length of matched string)

$scope.response = ""

matches = []
selectedMatch = null

# Outer loop - loop through every answer set (index 0 is always [All Other Answers] )
for i in [0...$scope.q_data.answers.length]

Copy link
Contributor

@cayb0rg cayb0rg Jun 6, 2024

Choose a reason for hiding this comment

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

Just noticed we need to reset response to the original response at the start of each loop iteration. There's an edge case here where if a preceding answer doesn't require case sensitivity, then response will be lowercase forever, breaking the case sensitivity option for following answers. Same happens for character sensitivity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch, fixed!

@@ -439,8 +435,6 @@ angular.module('Adventure', ['ngAria', 'ngSanitize'])
# Loop through each match to see if it matches the recorded response
for j in [0...$scope.q_data.answers[i].options.matches.length]


# TODO make matching algo more robust
match = $scope.q_data.answers[i].options.matches[j]

# Remove whitespace
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another error irrelevant to the PR's changes, but would be good to include. The following code is removing special characters in addition to whitespace, leaving only (a-z, A-Z), digits (0-9), and underscores (_), so the character sensitivity option doesn't work properly. We should probably use the replace() function instead with the \s metacharacter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your suggestion appears to do the trick, fixed.

@cayb0rg
Copy link
Contributor

cayb0rg commented Jun 6, 2024

New changes look great, and partial match priority selection seems to work correctly. Apart from the issues with case/character sensitivity, this is looking good to me!

@clpetersonucf
Copy link
Member Author

Updated again: Short Answer matches should be more robust based on fixing issues @cayb0rg identified.

@clpetersonucf clpetersonucf requested a review from cayb0rg June 13, 2024 19:18
Copy link
Contributor

@cayb0rg cayb0rg left a comment

Choose a reason for hiding this comment

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

Case and special character sensitivity works great! Not spotting any other outstanding bugs, apart from what Matt found. Nice work 💪🏻

Copy link
Contributor

@FrenjaminBanklin FrenjaminBanklin left a comment

Choose a reason for hiding this comment

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

All of the listed features appear to be working well.

@dmols dmols self-requested a review June 18, 2024 21:12
Copy link

@dmols dmols left a comment

Choose a reason for hiding this comment

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

Tested it before the new changes, then after. Widget works as intended and the changes look good. Great work!

@clpetersonucf clpetersonucf merged commit e20eaf7 into ucfopen:master Jun 25, 2024
1 check passed
@clpetersonucf clpetersonucf deleted the feature/score-screen-and-fixes branch June 25, 2024 17:38
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.

4 participants