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

Visualiser: Begin Paused v2 #1052

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Visualiser: Begin Paused v2 #1052

wants to merge 1 commit into from

Conversation

Robadob
Copy link
Member

@Robadob Robadob commented Mar 25, 2023

Tweaks visualiser begin paused behaviour, so that it begins paused on the first step with agents.

The changes so far, cause it to pause the step after agents are created (if created in a step function).

The problem is twofold:

  • when stepExitConditions() triggers buffer update, for some reason it passes step_count+1.
  • It actually pauses on the next iteration (hence at the end of step 11), however before new buffer data has been passed (as that's where the pause mutex gets stuck).

Looking at git blame, the +1 occurred with this commit from @ptheywood, where step count update was moved to the end of step.

So not sure whether it wants to be removed or not, needs discussion.

Closes FLAMEGPU/FLAMEGPU2-visualiser#122

@Robadob Robadob self-assigned this Mar 25, 2023
@ptheywood
Copy link
Member

From that commit, looks like that change was to maintain the same value as before, while moving the step_count incrementing (deleted from line 73), so that the step_count is the last thing updated within the step method, making the the value is "correct" within an exit condtion.

This shouldn't have changed the value output to during the vis at all. It just means the timestep shown in the UI is the time after the current step, not the start of the current step. Removing the +1 would probably work, but then it would show 0 before step 0 has executed (if this would ever actually be visible) and at the end of step 0.

@ptheywood ptheywood marked this pull request as ready for review April 21, 2023 10:36
Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

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

This does cause the simulation to pause reliably at insertion time (with the count in the bottom right reading +1 of the iteration used internally).

It still isn't technically beginPaused though, so adding a docstring change to setBeginPaused noting this might be justified?

I.e. this is an improvement on the current behaviour so worth merging, but it is arguably not a full fix for beginPaused.

Happy for this to be merged with a docstring change, but not close any issues for this (if it's still even open), and will still need the workaround of having atleast one agent being rendered from step 0 for true begin paused.

@Robadob
Copy link
Member Author

Robadob commented Apr 21, 2023

I honestly don't see the value in pausing before agents. But sure, will get around to it when I have time, need to remind myself of all these changes.

@ptheywood
Copy link
Member

Yes, grand scheme it's unintersting and I'm being pedantic, but for e.g. recording a video of a rate-limtied simulation it might be of interest.

I don't think it's an urgent need, as there's a known workaround, so just a docblock change so the method is clearly described as pausing on the first iterations agnets are present would be fine for the immediate future.

Unsure if removing the two +1 being passed to the vis would cause other issues, or if it would just change the number shown.

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.

Begin Paused without Agents prior to Step 0
2 participants