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

Pulling methods out of struct definitions #54

Closed
mrufsvold opened this issue Jan 26, 2024 · 6 comments
Closed

Pulling methods out of struct definitions #54

mrufsvold opened this issue Jan 26, 2024 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@mrufsvold
Copy link
Contributor

Currently, much of the logic is stored in anonymous functions within struct definitions by overloading getproperty. This does allow OOP style code with scene.method() calls but it has two drawbacks:

  1. Stack traces contain anonymous functions with uninformative names. It would be nice to see init(::Main) in the stacktrace instead of a gensymed string of characters.
  2. Julia is designed for a more functional style so you're probably going to see a performance penalty for always having to go through the if s == :init ... elseif.. elseif... on every get property call. Using normal method calls allows multiple dispatch to do its thing and resolve the correct function, inline where appropriate, etc.

You could solve the first problem by just pulling these anonymous functions out, giving them names, and then still using get_property to dispatch to the correct function. Or you could go all the way and remove the OOP style and mayb see a performance benefit.

Anyway, I'm happy to do the work, but I wouldn't want to do such a major refactor without checking on the direction you'd like to go.

@Kyjor
Copy link
Owner

Kyjor commented Jan 26, 2024

This is something I would like to do very soon. I have some major changes I'm going to merge today. So just after that is fine. When it's done, I'll personally test it since we don't have complete code coverage yet. Thank you!

@Kyjor Kyjor added the enhancement New feature or request label Jan 26, 2024
@mrufsvold
Copy link
Contributor Author

Great! Just ping me when you think you're ready. I came up with a pretty good system for reorganize the code last night, so I should be able to get it done quickly!

@Kyjor
Copy link
Owner

Kyjor commented Jan 26, 2024

Sounds good! Just added some more automated testing as well so that should help a bit. After I merge that, I'll work on wrapping up the big change set and let yo u know. Should be about 30 minutes!

@Kyjor
Copy link
Owner

Kyjor commented Jan 26, 2024

Just merged! You should be good to go. Thank you!

@mrufsvold
Copy link
Contributor Author

The functions are moved out in #57 ! It'll take a lot longer to find every function call and reorder it, but that can be more incremental as different parts of the code base get updates!

@Kyjor
Copy link
Owner

Kyjor commented Jan 27, 2024

Thank you!

@Kyjor Kyjor closed this as completed Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants