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

Feature/maroon 465 reusable ode solver #478

Merged
merged 86 commits into from
Jul 17, 2024

Conversation

jonathanplatzer
Copy link
Collaborator

@jonathanplatzer jonathanplatzer commented May 7, 2024

Implementation of a reusable solver for ODEs describing motion in 3D.
Closes #465

@jonathanplatzer jonathanplatzer self-assigned this May 7, 2024
Copy link
Collaborator

@FlorianGlawogger FlorianGlawogger left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, looks great; please see my responses to the comments above. I.e. I think there are now 3 things left: delete Tests.cs file, re-add and fix camera zoom, and change rocket launch background to grass field and add rocket model. 👍
and please merge with the develop branch (Github Actions tests also need to pass)

@jonathanplatzer
Copy link
Collaborator Author

jonathanplatzer commented Jul 8, 2024

Ok, this is rather weird, before merging develop only the playmode tests were failing, but now after the fact also the WebGL build fails... Also the error log is rather unhelpful, I searched for errors that my code could have caused, but haven't found anything that seems related

edit:
Ok, seems like the WebGL build had just some hiccup, i re-run it and it passed. Also some good news, I figured out which test case failed, and of course it is caused by my code.
Turns out searching for "error" in the log is not that helpful, but when while looking through all occurrences of "failed" i found the error. It seems like while restructuring the 3D Motion Experiment I inadvertently left out/removed some NullReferenceException handling.

Copy link
Collaborator

@FlorianGlawogger FlorianGlawogger left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your changes again 👍 the zooming out part now works very well! :)
I juts noticed there's a NullReferenceError when selecting the Coupled Oscillators parameters, and also maybe you can try to reduce the file size of the rocket model? (Modifying assets from the Unity Asset store is allowed) (please see my other comment regarding that)
Otherwise the PR looks very solid now, and I think when these 2 issues are fixed, I think we'll finally be able to merge it 😄 (sorry again about the long review process)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I select Coupled Oscillators, I get a NullReferenceException:
grafik

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good catch, i wanted to make background and particle optional parameters in the JSON experiment files, which worked fine at first, but it seems there was a regression. I think everything should be fixed by using null-conditional when referencing the optional fields

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since all of the textures combined are 37MB, maybe it's possible we can compress them/reduce size? I know I suggested this asset, so I'm sorry this is now again a bit of work to do; but maybe you could either scale down the textures to maybe 256x256 and see if it still looks fine? Maybe we can fit all textures within a ~2MB limit or something like that? I think since the spaceship will not be viewed from close up that should still be fine?
I talked with Michael, and he said otherwise you can also delete e.g. the normal maps, or any other textures besides the color textures depending on how scaling down the textures goes. If it's too much work required, you can also delete the textures and just assign a red material to the spaceship body, and a gray-ish metallic material to the landing gear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have removed every texture besides the albedo textures, and resized them to a more reasonable 1024x1024. Now the whole thing is a much more reasonable 350KB instead of 37MB, and it looks exactly the same, so that's that...

Copy link
Collaborator

@FlorianGlawogger FlorianGlawogger left a comment

Choose a reason for hiding this comment

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

Rocket model still looks good and the new file size is much more reasonable, coupled oscillators parameter preset now also works again!
tldr: Looks good now, thanks 👍

@FlorianGlawogger FlorianGlawogger merged commit 5f3494f into develop Jul 17, 2024
6 checks passed
@FlorianGlawogger FlorianGlawogger deleted the feature/Maroon-465_ReusableODESolver branch July 17, 2024 08:00
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.

Reusable 3D ODE solver
3 participants