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

Implemented ReflectiveShell as optional boundary #512

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ehlertdo
Copy link

Hi,
I needed a reflective boundary in my 3D simulations but in the form of a sphere instead of the currently available ReflectiveBox. Maybe this feature could be of more general interest since astrophysical environments are often more spherical than they are boxy.
Note that the shell reflects in both directions, i.e. outside particles are kept out and inside particles are kept in.
Best,
Domenik

PS: I copied the distance() and normal() functions from the Sphere class in Geometry.cpp but probably there's a more elegant way to do this with inheritance.

cr_tracks
cr_tracks

Copy link
Member

@JulienDoerner JulienDoerner left a comment

Choose a reason for hiding this comment

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

Hey @ehlertdo,

Thanks for sharing your implementation. In general, the tool looks good for me.

I have tested it and found some deviation when using large steps. I guess this is caused by the position where you calculate the normal. This is given at the current position which is not necessarily a position on the sphere. Would you have an idea how to fix this, or can this be neglected in regular cases?
image

Beside this I would ask you to add your changes to the Changes.md and maybe you can provide a simple test case as a notebook and as unit test.

@JulienDoerner JulienDoerner self-assigned this Feb 26, 2025
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