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

[celx] Implement object:setatmosphere with a table instead of 22 numbers #1938

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

375gnu
Copy link
Collaborator

@375gnu 375gnu commented Oct 15, 2023

+ small cleanup in object:mark.

I doubt that anybody really used method with 22 arguments, so I hope it's safe to change API.

@375gnu 375gnu force-pushed the object_setatmosphere branch 2 times, most recently from 5365ca5 to 29a8d08 Compare October 15, 2023 19:37
@375gnu 375gnu force-pushed the object_setatmosphere branch from 29a8d08 to 311b396 Compare October 15, 2023 19:39
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@375gnu
Copy link
Collaborator Author

375gnu commented Oct 15, 2023

I also don't like non-const Body::getAtmosphere(). I'd prefer const version only and instead of direct edit of a body's atmosphere do thee following:

Atmosphere atmosphere;
if (body->getAtmosphere() != nullptr)
    atmosphere = *body->getAtmosphere();
...
body->setAtmosphere(atmosphere);

Any thoughts?

One downside is a little bit decreased performance due to initialization and copying objects. But I doubt it will be noticeable.

@ajtribick
Copy link
Collaborator

I did originally have a version that replaced the atmosphere instead of editing it, but I'm not sure if anyone's using this to do animations. It's also not clear to me why this method doesn't allow creating an atmosphere if one doesn't exist.

@375gnu 375gnu merged commit 32e66a9 into master Nov 6, 2023
@375gnu 375gnu deleted the object_setatmosphere branch November 6, 2023 14:23
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.

2 participants