Skip to content

Deprecate or document/test SCENES_IN_ORDER #74

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

Closed
leotrs opened this issue May 24, 2020 · 2 comments
Closed

Deprecate or document/test SCENES_IN_ORDER #74

leotrs opened this issue May 24, 2020 · 2 comments
Labels
pr:deprecation Deprecation, or removal of deprecated code

Comments

@leotrs
Copy link
Contributor

leotrs commented May 24, 2020

The function get_scene_classes_from_module in extract_scene.py checks for a module member called SCENES_IN_ORDER. It appears that the user is allowed to assign a list to this that specifies the order in which the scenes will be rendered.

  1. Given that there are plans to render scenes in parallel, this would become deprecated.
  2. If we continue to support rendering scenes in series for some purpose, I'm still not sure what is the purpose of determining the rendering order.
  3. Even if we wanted to do so, it seems more natural that the rendering order should be the order in which scenes appear in the module, rather than setting a separate variable for the order.
  4. In any case, if the SCENES_IN_ORDER variable will still be used, we should document it somewhere. If it won't be used, we should delete it. (Lines 124 and 125 in extract_scene.py are the only mentions to this variable in the code base.)
  5. Finally, if we do choose to support it, we should write a test for it.
@leotrs leotrs changed the title Document SCENES_IN_ORDER Deprecate and/or document SCENES_IN_ORDER May 24, 2020
@leotrs leotrs changed the title Deprecate and/or document SCENES_IN_ORDER Deprecate or document/test SCENES_IN_ORDER May 24, 2020
@XorUnison
Copy link
Collaborator

XorUnison commented May 24, 2020

Although we should implement multiprocessing, I would add this as an option, as opposed to replacing the current linear render process for reasons I'll mention in the new issue (#80).

As for removing this... I mean it is an overly specific use case, yes. I do struggle to think of any good reasons to keep it.
The vast majority of people will have enough CPU power to at least benefit from multiprocessing a bit, and a scene order just doesn't do a whole lot for people unless they're sitting anxiously on the edge of their seat for every single result.

Anyone have a better case to make for why to keep this?

@PgBiel PgBiel added the pr:deprecation Deprecation, or removal of deprecated code label May 26, 2020
@leotrs
Copy link
Contributor Author

leotrs commented May 27, 2020

I don't hear anyone making arguments for keeping this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:deprecation Deprecation, or removal of deprecated code
Projects
None yet
Development

No branches or pull requests

3 participants