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 option to create geometric bodies (spheres, prisms, ellipsoids, etc) and compute their effect. #4583

Merged
merged 14 commits into from
Dec 18, 2020

Conversation

joa-quim
Copy link
Member

Tested for a couple of them (bodies) but not all.

Copy link
Member

@PaulWessel PaulWessel left a comment

Choose a reason for hiding this comment

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

You cannot break the +modifier syntax since we will auto-compute these from long-forms, eventually. Things like -M+bell is not a modifier since bell is a word. It has to be, e.g., -M+sbell where +s means shape. We can then later handle -Mshape=bell in long-form and auto-convert to -M+sbell. So need to be +scylinder (see typo), +scone, +sprism, etc.

Type Alaternative.

@joa-quim
Copy link
Member Author

OK, -M+ssphere it is.

Copy link
Member

@PaulWessel PaulWessel left a comment

Choose a reason for hiding this comment

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

You need to merge master into this first, no. Otherwise, I see you are reverting several things in master related to recent bug fixes, e.g., in gmt_io.c, grdcut.c, grdedit.c.
Your test script is missing the **+**s modifier.
The listing of bell,height, etc should be italics not bold since these are parameters, and you have several references to instead of height in that section (i.e., there are lots of these). Pirimid -> Pyramid.

@joa-quim
Copy link
Member Author

I had merge master but had conflicts and ended up no committing all master changes in this branch. Had also forgotten to commit the cube_mag.sh test update to +s.

doc/rst/source/supplements/potential/gmtgravmag3d.rst Outdated Show resolved Hide resolved
doc/rst/source/supplements/potential/gmtgravmag3d.rst Outdated Show resolved Hide resolved
doc/rst/source/supplements/potential/gmtgravmag3d.rst Outdated Show resolved Hide resolved
doc/rst/source/supplements/potential/gmtgravmag3d.rst Outdated Show resolved Hide resolved
src/potential/solids.c Outdated Show resolved Hide resolved
src/potential/solids.c Outdated Show resolved Hide resolved
@joa-quim
Copy link
Member Author

Ping

@PaulWessel
Copy link
Member

Think you have to do all those Mimi commits

@PaulWessel
Copy link
Member

Mini

@seisman
Copy link
Member

seisman commented Dec 18, 2020

@joa-quim manually fixed the typos in the commit 70c1196.

You don't have to do that. You can just click the "Commit suggestions" button.

@seisman
Copy link
Member

seisman commented Dec 18, 2020

Ping @joa-quim

Following bash scripts may not have execute permission:
1: ./test/potential/cube_mag.sh

@joa-quim
Copy link
Member Author

They are committed.

@PaulWessel
Copy link
Member

OK, I guess you did not use the buttons we set up for you to click commit as they all show up there, so harder to see if they were fixed. Also, per @seisman the script does not have executable permission so please fix that so we can approve.
Sorry, slow day with family insisting on morning swim

@joa-quim
Copy link
Member Author

The script has the +x permission on my side. But the commit doesn't believe it.

@PaulWessel
Copy link
Member

OK, will fix if needed.

@joa-quim joa-quim merged commit cd391ad into master Dec 18, 2020
@joa-quim joa-quim deleted the gmtgravmag3d-bodies branch December 18, 2020 22:48
@seisman
Copy link
Member

seisman commented Dec 19, 2020

The script has the +x permission on my side. But the commit doesn't believe it.

FYI, I added the +x permission to the script and pushed the change directly to the master branch.

@seisman
Copy link
Member

seisman commented Dec 19, 2020

@joa-quim This PR introduces some compiler warnings:

../../src/potential/gmtgravmag3d.c:243:7: warning: unused variable 'ptr' [-Wunused-variable]
        char ptr[GMT_LEN256] = {""};
             ^
In file included from ../../src/potential/gmtgravmag3d.c:667:
../../src/potential/solids.c:37:3: warning: '/*' within block comment [-Wcomment]
                /* first triangle */
                ^
../../src/potential/solids.c:108:1: warning: '/*' within block comment [-Wcomment]
/*      as a union of triangular facets. Returns number of triangles. */
^

@seisman seisman mentioned this pull request Dec 19, 2020
4 tasks
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.

None yet

3 participants