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

magic numbers in CartoonModelList #186

Closed
pixelzoom opened this issue Nov 9, 2015 · 6 comments
Closed

magic numbers in CartoonModelList #186

pixelzoom opened this issue Nov 9, 2015 · 6 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

Related to code review #173.

There are numerous magic numbers in CartoonModelList.

Issues:

• Some of these constants are documented as "tuned by hand", some are not documented. All magic numbers should be documented, along with any dependencies that other things may have on this numbers. (Eg, "If you change foo, be sure to change bar.")

• Some are duplicated in multiple places. For example this.sun.radius *= 50 appears in 2 places. If I change one, do I need to change the other? Similarly for this.earth.radius *= 800.

this.timeScale = 365.0 / 334.0 is odd. Is there a significance to the numerator and denominator values? If so, document. If not, collapse to a single number.

@aaronsamuel137
Copy link
Contributor

Factored out some of these into constants, and documented as best I can without knowing for sure where these came from (ported directly from Java with all comments).

Regarding 365.0 / 334.0, the 365 seems like is must refer to days in a year, but I am not 100% sure where these numbers came from. I am inclined to leave it as is. Perhaps @samreid has some memory of this?

@samreid
Copy link
Member

samreid commented Nov 12, 2015

This is fuzzy in my mind, but I recall manual adjustment of values was necessary to produce the desired macroscopic behavior. The code comment in the context is:

    // Have to artificially scale up the time readout so that Sun/Earth/Moon mode has a stable orbit with correct
    // periods since masses are nonphysical
    this.timeScale = 365.0 / 334.0;

@aaronsamuel137
Copy link
Contributor

Assigning to @pixelzoom for review.

@pixelzoom
Copy link
Contributor Author

@samreid In #186 (comment), is 365.0 indeed the number of days in a year?

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Nov 16, 2015
@samreid
Copy link
Member

samreid commented Nov 16, 2015

In this model, yes.

pixelzoom added a commit that referenced this issue Nov 16, 2015
@pixelzoom
Copy link
Contributor Author

Thanks. Noted in source code doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants