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

Add convenient constructor for Torus #285

Merged
merged 13 commits into from
May 12, 2022

Conversation

SebastianRuffert
Copy link
Contributor

No description provided.

@SebastianRuffert SebastianRuffert changed the title Torus constructor Add convenient constructor for Box Apr 25, 2022
@SebastianRuffert SebastianRuffert changed the title Add convenient constructor for Box Add convenient constructor for Torus Apr 25, 2022
@fhagemann
Copy link
Collaborator

fhagemann commented Apr 25, 2022

It might also make sense to allow φ to be a Tuple and do the conversion to a T in the constructor (see #240).
Or to generally allow for the same keywords as we do when parsing a dictionary from a config file (?)

When no type is explicitely passed to the `Torus` constructor, it should take the promoted type only from the keyword arguments. Thus, the default parameter are defined as `Int`. If all parameters are `Int`, the `Torus` methods that perform the type promotion ensure that the `Torus` is always of type `Float`.
@fhagemann fhagemann requested a review from lmh91 May 3, 2022 10:46
@lmh91
Copy link
Collaborator

lmh91 commented May 10, 2022

I just realized that we have two rotations in the constructor due to φ.
The order how they are applied is important.
I think we did not specified that in the documentation so far, did we?

rotation * RotZ(φ[1]) != RotZ(φ[1]) * rotation is the "issue".

We will have to decide for one and stick to it for all primitives and have to state it in the documentation.

Any opinions on which order is the better one? In sense of that it is easier to construct certain geometries.

@SebastianRuffert
Copy link
Contributor Author

I just realized that we have two rotations in the constructor due to φ. The order how they are applied is important. I think we did not specified that in the documentation so far, did we?

rotation * RotZ(φ[1]) != RotZ(φ[1]) * rotation is the "issue".

We will have to decide for one and stick to it for all primitives and have to state it in the documentation.

Any opinions on which order is the better one? In sense of that it is easier to construct certain geometries.

I think we should stick to rotation*RotZ(φ[1]). The user wants to construct a specific geometry ranging from one polar angle to the other one, afterwards he rotates it. Thus this order.

@lmh91
Copy link
Collaborator

lmh91 commented May 10, 2022

I just realized that we have two rotations in the constructor due to φ. The order how they are applied is important. I think we did not specified that in the documentation so far, did we?
rotation * RotZ(φ[1]) != RotZ(φ[1]) * rotation is the "issue".
We will have to decide for one and stick to it for all primitives and have to state it in the documentation.
Any opinions on which order is the better one? In sense of that it is easier to construct certain geometries.

I think we should stick to rotation*RotZ(φ[1]). The user wants to construct a specific geometry ranging from one polar angle to the other one, afterwards he rotates it. Thus this order.

Sounds reasonable.
Could please make a note into the documentation in /docs/src/man/csg.md?
Maybe under ### Combination of Transformation?

With some example maybe? Or extend the example of the tube by adding also some limitation in phi?

```yaml
tube:
r: 1
phi: (90,180)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that works.
One has to use the from-to syntax in the configuration files.

I think this note here should be enough:

!!! note
    If $\varphi$ is specified for a certain interval, the interval is internally converted into a rotation, $R_{\varphi}$, of the primitive.
    This rotation is applied prior to the rotation specified in the rotation field, $R$. 
    Thus, internally, both rotations are calculated into the final rotation matrix of the primitive, $R_f$, via $R_f = R \cdot R_{\varphi}$.    

Could you please add this unter the Rotation section
and just delete this part. I think the above note is enough.

@lmh91 lmh91 merged commit 0c8c1ae into JuliaPhysics:main May 12, 2022
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.

3 participants