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

Remove variable frameloops #216

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

Remove variable frameloops #216

wants to merge 1 commit into from

Conversation

robalni
Copy link
Contributor

@robalni robalni commented Oct 4, 2020

It was hard to understand what it meant and it was only used for
determining whether the loop was in the first frame/iteration.
Therefore I replaced it with variable first_frame which has a clearer
purpose.

It was hard to understand what it meant and it was only used for
determining whether the loop was in the first frame/iteration.
Therefore I replaced it with variable `first_frame` which has a clearer
purpose.
Copy link
Contributor

@MoonPadUSer MoonPadUSer left a comment

Choose a reason for hiding this comment

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

Looks good to me, please wait for another approval tho

@TheAssassin
Copy link
Member

The purpose of the new variable is indeed clearer. However, I ask myself, can't we just get rid of these ifs completely? Perhaps by moving the stuff outside the ifs before the loop (risking some code duplication, but I could imagine not all calls might be needed in the first frame anyway)?

@voidanix
Copy link
Member

voidanix commented Jan 9, 2023

I think the question should really be what is the actual purpose of frameloops aka first_frame?

Removing the variable with its if-statements altogether results in the exact same behavior; plus, considering the int frames parameter does absolutely nothing, it could very likely be a remnant from old code.

If this can be dropped without regressions, then better take that route.

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