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

Update cart2polar.glsl #236

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

Conversation

jane00
Copy link
Contributor

@jane00 jane00 commented Dec 7, 2024

Add description for convention changed.

Add by jane00
Refer to https://en.wikipedia.org/wiki/Spherical_coordinate_system , there are two different definition for phi and theta, which are opposite to each other.

Before the changes by shadielhajj committed on 2024 Jul 26, The physics convention(ISO convention) is followed, phi means "azimuthal angle", theta for "polar angle"
but now, The "mathematics convention" is followed, phi means "polar angle", theta for "azimuthal angle"

So be careful with the old code !!!

Add description for convention changed.
@patriciogonzalezvivo
Copy link
Owner

@jane00 feels like this description belongs to the 'description' section on the YAML formatted header (in comments). And should be added to: 'cart2polar.glsl', 'cart2polar.hlsl', 'cart2polar.msl' and also 'polar2cart.glsl', 'polar2cart.hlsl' and 'polar2cart.msl'

@jane00
Copy link
Contributor Author

jane00 commented Dec 10, 2024

@jane00 feels like this description belongs to the 'description' section on the YAML formatted header (in comments). And should be added to: 'cart2polar.glsl', 'cart2polar.hlsl', 'cart2polar.msl' and also 'polar2cart.glsl', 'polar2cart.hlsl' and 'polar2cart.msl'

Sorry, I'm not very familiar with the correct format conventions, so my submission might not be very standardized. However, I happened to use this function. I originally wanted to add a "vec3 polar2cart(in vec3 polar)" function myself, but I found that the new version already includes it. Then I noticed the definition had changed, so I added a note about it.

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