Repository navigation
Make sequence puzzle hint signs optional - #2924
Conversation
|
Test build no longer available. |
|
Hola! He estado probando los cambios que hiciste y he visto que funcionan correctamente. En todo caso recomiendo que resuelvas el check de formateo del codigo, la forma mas sencilla es instalar pre-commit como se indica aqui. |
|
@Un-player0 thank you so much for the peer review! @BiancaPierina let us know when it's ready for review again. |
- Removed the editor warning that required a hint sign. - Added is_solved to SequencePuzzleStep. - Updated SequencePuzzle to use step-level solved state. - Added null guards around all hint_sign access.
08d7144 to
1a4965c
Compare
|
@BiancaPierina this still has formatting issues, as @Un-player0 mentioned. They are shown here: https://github.com/endlessm/threadbare/actions/runs/35646980876/job/106489712011?pr=2924 But the easiest thing you can do is to install pre-commit, which will automatically fix them for you. Check https://github.com/endlessm/threadbare/wiki/Contributing#coding-style |
|
@BiancaPierina I commited the formatting fixes myself this time. Also I removed the hint sign from After The Tremor: 
And this shows that your solution works! I tested |
| @@ -80,7 +82,7 @@ func _update_current_step() -> void: | |||
| var previous_step := _current_step | |||
| for i in range(_current_step, steps.size()): | |||
| # We find the next fire that is not solved, and that's the _current_step | |||
There was a problem hiding this comment.
The comment is outdated. The "next fire" was referring to the hint (a bonfire in the main game).
| # We find the next fire that is not solved, and that's the _current_step | |
| # We find the next step that is not solved, and that's the _current_step |
| if steps[i].hint_sign.is_solved: | ||
| if steps[i].is_solved: |
| ## Whether this step has been solved. | ||
| var is_solved: bool = false |
There was a problem hiding this comment.
Yes I think it's good adding this to the step. The hint needs it for showing different animations (idle or solved). I wonder if we can get rid of this duplication by passing the step to the hint.
This can be an @export. And in that case I would add it above the optional hint_sign export.
| if step.hint_sign: | ||
| step.is_solved = step.hint_sign.is_solved | ||
| step.hint_sign.demonstrate_sequence.connect(_on_demonstrate_sequence.bind(step)) |
There was a problem hiding this comment.
| if step.hint_sign: | |
| step.is_solved = step.hint_sign.is_solved | |
| step.hint_sign.demonstrate_sequence.connect(_on_demonstrate_sequence.bind(step)) | |
| if step.hint_sign: | |
| step.hint_sign.demonstrate_sequence.connect(_on_demonstrate_sequence.bind(step)) |
I don't think that the step is_solved should be set to the hint_sign is_solved here.
|
@BiancaPierina this time I did the changes myself in order to merge. Thank you! |
|
Thank you so much for taking care of the final changes and merging the pull request! I did try to run pre-commit in Git Bash, but the manual command didn’t work. I was planning to ask my professors for help in class today. |
3 participants
Closes: #2149