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

Chart editor play(state)test to results screen #4087

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

Lasercar
Copy link
Contributor

@Lasercar Lasercar commented Feb 3, 2025

Does this PR close any issues? If so, link them below.

#4075

Briefly describe the issue(s) fixed.

Added an option to show the results screen after a play(state)test of a chart. It also calculates the clear percentage from the starting time.

Include any relevant screenshots or videos.

Initial version
2025-02-03.20-12-34.1.mp4
Latest version
2025-02-08.01-31-38.1.mp4

This change is very basic at the moment, it could probably use some options in the chart editor (like, whether to even show the results screen in the first place, and if it should only show it if more than 50-60% of the song is played). There also might be some other issues that I haven't noticed yet. Finished!

I didn't actually expect this to actually work. I guess it just goes to show how great the new engine is (except for the fact that I had to stop the music from being replaced by the results music as it caused issues with the chart editor).

Change also requires this PR for the assets FunkinCrew/funkin.assets#118 (otherwise the option won't be there to toggle, lol)

Oh and, just something of note, you actually have to play the song yourself to get the score - enabling bot mode disables this from doing anything.

I might investigate if I can fix the results screen music by making it play the music as a sound instead, like the how game over substate does it.

@github-actions github-actions bot added size: medium A medium pull request with 100 or fewer changes. pr: haxe PR modifies game code. status: pending triage Awaiting review. and removed size: medium A medium pull request with 100 or fewer changes. labels Feb 3, 2025
@AbnormalPoof AbnormalPoof added type: enhancement Involves an enhancement or new feature. topic: chart editor Related to the operation of the Chart Editor. size: small A small pull request with 10 or fewer changes. labels Feb 3, 2025
@Lasercar Lasercar force-pushed the chart-editor-playtest-results-screen branch from 2dcae45 to a25fbc9 Compare February 4, 2025 10:30
@github-actions github-actions bot added size: medium A medium pull request with 100 or fewer changes. and removed size: small A small pull request with 10 or fewer changes. labels Feb 4, 2025
@amyspark-ng
Copy link
Contributor

good good

@Lasercar Lasercar changed the base branch from main to develop February 5, 2025 10:56
very buggy and risky to use - moving your mouse over the chart before it's finished clearing up the textures will cause it to crash
@Lasercar Lasercar force-pushed the chart-editor-playtest-results-screen branch from a25fbc9 to 73cfe37 Compare February 5, 2025 10:56
@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 5, 2025

Also, I noticed while doing this that the pause screen button and the chart editor button always open a new chart editor state, rather than simply closing the playstate, which is weird, because it means that if you don't save before playtesting and go back to the charteditor by either method, you lose your work.

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 5, 2025

Maybe #2323 will be helpful to you

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 5, 2025

Ah. Well would you look at that, it's already fixed in the develop branch, even in the pause substate:
https://github.com/FunkinCrew/Funkin/blob/develop/source/funkin/play/PauseSubState.hx#L756

@Lasercar Lasercar force-pushed the chart-editor-playtest-results-screen branch from f28c450 to 73cfe37 Compare February 6, 2025 15:15
@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

Maybe I could only show the results screen if the completion is over, hmm, let's say, 30%.

Bonus feature - debug chart button opens current selected freeplay song
@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

I've also added a bonus feature - open the chart editor with the currently selected freeplay song. Moved to separate PR: #4114

@Lasercar Lasercar marked this pull request as ready for review February 7, 2025 07:52
@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

Ah crap, I got that weird issue again...
Screenshot 2025-02-07 190117
I started halfway in the song, so maybe it doesn't do this if you're at the start of the song (0.0 seconds?).
And this definitely only happens with this playtest to results screen change.

Ok, I tested it again, and it didn't happen, and then again, and it did?

Ah, clicking the to start button fixes it, though the playstate gets broken somehow.

@Lasercar Lasercar marked this pull request as draft February 7, 2025 09:21
@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

Hmm, well I encountered an issue with a tween trying to tween a null thing, so I've made it clear all the tweens too, and I think it's fixed? (Don't mind the video quality, it was the only way to make it small enough)

bit compressed video
2025-02-07.19-31-12.1.mp4

Ohhhh, I got the issue again, and now I know what it is lol:

video of bug
2025-02-07.19-41-22.mp4

I had no idea that the chart editor actually worked that way!
Now I just have to figure out how to fix it...

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

prevented the results music from playing
2025-02-07.23-18-59.1.mp4

Wait I second, I didn't add this as an option yet. done now

@Lasercar Lasercar marked this pull request as ready for review February 7, 2025 13:22
@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

Hmm, has this already been reported?
Screenshot 2025-02-07 234611

@amyspark-ng
Copy link
Contributor

amyspark-ng commented Feb 7, 2025

is it possible for the max amount of notes to come from the amount of notes that exist since you press enter until the song ends rather than the full real amount of notes?

this way the tally will feel much more accurate

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

Uh.... I'm not sure how I'd go about doing that. I'll have a look at trying to do it though.

@github-actions github-actions bot added size: large A large pull request with more than 100 changes. and removed size: medium A medium pull request with 100 or fewer changes. labels Feb 7, 2025
@amyspark-ng
Copy link
Contributor

Uh.... I'm not sure how I'd go about doing that. I'll have a look at trying to do it though.

guessing it's hard, i know results state probably takes a tally as a parameter but i guess you'd have to create a whole new tally

i say this because let's say you only test a song from half down, you'd have "missed" half the notes and get a bad rank, even though you only played half

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

Alright, I think I found the spot.

Now it's time to test to see if it works.

Looks like I'll be coding for a little while more - I noticed that the startTimestamp isn't passed into the regenNoteData, so my code to remove a total note for each note before the startTimestamp doesn't work.

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

Though now that I've passed it in (and done some more testing and coding), it now works!

video
2025-02-08.01-31-38.1.mp4

Of course, if you do funky stuff like flipping the strumlines (like what happens in that mind games mod that I'm a big fan of), you'd have to account for that in your own code.

@amyspark-ng Check it out! Your feature request is now fully implemented!

Oh and, just something of note, you actually have to play the song yourself to get the score - enabling bot mode disables this from doing anything.

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 7, 2025

I could probably make the toggle for bot mode disable the show results toggle and vice-versa, but that might confuse some people who don't know that they're mutually exclusive so I'll just leave it as is unless one of you reviewers object to it (like some kind of ace attorney).

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 7, 2025

Why did Tutorial end early, and why did the results screen music not play?
Did you time travel to the end of the song?

@AbnormalPoof
Copy link
Collaborator

Why did Tutorial end early, and why did the results screen music not play? Did you time travel to the end of the song?

Debug feature most likely. You can press 1 on debug builds to immediately end the song.

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 7, 2025

Great, thank you for clarifying.
I'd still like an explanation for the muted Results screen though, that should be checked out!

@AbnormalPoof
Copy link
Collaborator

(except for the fact that I had to stop the music from being replaced by the results music as it caused issues with the chart editor)

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 7, 2025

Oh my gosh, I completely missed that
Thanks for digging that up!

@Lasercar, please keep your comments concise and informative. Much of your discussion involves thoughts and details we don't need to know, which floods the PR and obscures useful information.

@amyspark-ng
Copy link
Contributor

awesomeeee

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 8, 2025

@Lasercar, please keep your comments concise and informative. Much of your discussion involves thoughts and details we don't need to know, which floods the PR and obscures useful information.

I get that, but I'd rather that people understand exactly why I do as I do and say what I say. For example, just the other day, there was the whole thing with the comments, if I had originally mentioned why I did the comments as I did, then it would of avoided all that confusion. If I had linked that comment directly, you wouldn't of had to ask where it was from, and if I had linked the style guide directly when I mentioned, it would of solved that confusion as well.

We aren't exactly in the same timezones either, so by the time I notice and get a chance to respond, it's probably already been a few hours.

That all said though. If you miss still end up missing the information despite my attempts at being as clear as possible, then I can't really help you with that aside from 1. linking the part where I said it and 2. asking you to be more careful and reading everything (because that one has gotten me more than a few times by now, and it's always really annoying when it happens - what's that, I got a problem? silly me spent 5 hours trying to solve it only to find out that he didn't read the part of the instruction that prevents what he got from happening).

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 8, 2025

My entire point was that you include such an overwhelming amount of information that it is impractical for me to read through every comment and additional edit that you make. Readers shouldn't be expected to check whether you've added multiple new edits every time you make a reply.

I have asked you multiple times to include less unnecessary information to improve clarity, but you have continued to type so much that I can't find what you mentioned about a specific thing after a substantial amount of searching.

I greatly appreciate your many contributions to the community and have tried my best to communicate my feedback to help you improve your work.

I only ask you questions because I wish to understand what features you are implementing in your PRs. Despite this, you have often replied to myself and others in an uncooperative and disrespectful manner.

Please respond to feedback in a respectful and constructive manner moving forward. I have been really impressed with your contributions thus far and would love to see your work pay off!

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 8, 2025

I only ask you questions because I wish to understand what features you are implementing in your PRs. Despite this, you have often replied to myself and others in an uncooperative and disrespectful manner.

Please respond to feedback in a respectful and constructive manner moving forward. I have been really impressed with your contributions thus far and would love to see your work pay off!

Ahh, I'm sorry that you see it that way. When you see me type text like that, it's because I'm actually irritated at myself (for causing you/whomever getting irritated at me), not you or whomever I'm responding to.

It's really hard to get that across with only text.

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 8, 2025

Regardless of your cause for irritation, please do not reply to others in an impatient manner.

When someone misses a detail or an answer, kindly link the answer in a reply.

We're here to help each other learn and improve our work in this community.

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 8, 2025

Sure! If you see it happening again and see it like that, point it out on me, but in a way that won't cause me to get irritated at myself and make it worse (say something like "you're doing the thing again"), that way I can apologise for it, analyse it, and figure out a way to respond better/properly.

@Lasercar
Copy link
Contributor Author

Lasercar commented Feb 8, 2025

OH, I just had a brilliant idea - what if I put all my excess thoughts/stuff into a discussion, rather than in the PRs here? That'd solve that problem too? Yeah?

I'd make a new comment for each PR, and then reply to it with the stuff related to that PR. If someone has a comment relating to one of those things, they'd make the post in the discussions rather than here and clogging up the PR.

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 8, 2025

Thank you for your cooperation.

I wanted to leave some feedback that applies to this and a few other PRs:

When you are fixing an issue (like the Results screen music breaking something), please do not remove existing features to get the game to work. Instead, find a solution to the problem that preserves all features.

In this PR, the ideal solution is to fix the returning to chart editor problem while preserving the Results screen music when playtesting.

In your screenshot plugin PR, the ideal solution is to fix the game's performance without introducing a major screenshot saving bug.

My priority when reviewing PRs is to ensure that the changes don't introduce major bugs and break the game. As long as you address those issues, your PR will be safe to implement into the game.

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 8, 2025

OH, I just had a brilliant idea - what if I put all my excess thoughts/stuff into a discussion, rather than in the PRs here? That'd solve that problem too? Yeah?

I'd make a new comment for each PR, and then reply to it with the stuff related to that PR. If someone has a comment relating to one of those things, they'd make the post in the discussions rather than here and clogging up the PR.

I see your reasoning, but I think you should keep your discussion in its related PR for accessibility.

My suggestion for reducing clutter is to weigh whether something is constructive or not before posting it.

The best way to keep the comments clean is to edit your original PR's description each time you have an update rather than uploading the video in a new comment each time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: haxe PR modifies game code. size: large A large pull request with more than 100 changes. status: pending triage Awaiting review. topic: chart editor Related to the operation of the Chart Editor. type: enhancement Involves an enhancement or new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants