arohl / gdis

A visualization program for the display, manipulation, and analysis of isolated molecules and periodic structures
GNU General Public License v2.0
43 stars 17 forks source link

Add option to scale balls according to vdW radii #41

Closed cajfisher closed 3 years ago

cajfisher commented 3 years ago

Ticking the "Scale ball and stick atoms" box in the Display Properties dialogue below "Antalias" scales atoms to vdW radii when in ball & stick viewing mode, both in the model display window and povray images. Also changes "ball_rad" and "stick_rad" to "ball_radius" and "stick_radius", respectively, for clarity and to be consistent with other variable names.

ovhpa commented 3 years ago

I have a little difficulty with that one: The scaling of atoms (ie the ball radius) with your option is made according to the vdW radius times the actual value writen in the "Ball radius" field.

actual radius = vdW Radius * Ball Radius

In that case it is unfortunately limited to 0.5 (the interval for the "Ball Radius" field) In that case, wouldn't it be better to use CPK scaling (which is scaling the radius of atoms from 0.1 to 3.0 times the vdW radius) for Ball & Stick directly? We can have the switch relabel to "Scale ball & stick atoms" to "use CPK scale for B&S" or something similar, and which will make B&S use the "CPK scaling" (= 0.1~3.0 times vdW radius) instead of "Ball Radius". The point is not to limit your vdW scaling idea (which is great) for B&S to the [0.1,0.5] interval of Ball radius.

That would also eliminate the rather confusing label "Scale ball & stick atoms".

What do you think about this?

ovhpa commented 3 years ago

PS: I forget to say : everything seems OK with your implementation but static analysis report the use of an undefined value in the file file_povray.c in the write_povray function, l. 425:

radius *= sysenv.render.ball_radius;

the static report is "The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage". Usually, setting an initial value (here for radius) at declaration is enough to get rid of such warning. Note that usually such warning are also not so important. ^^'

Best,

cajfisher commented 3 years ago

Oops, sorry about that initialization mistake. It should be fixed now. Yes, you're right about the scaling limitation, but I hadn't thought of it as a problem because above about 0.5 it becomes identical to the CPK model anyway. In any case, I've changed it to CPK scaling as suggested, but made the spinner increment the same as for the ball radius spinner to give the user closer control. See what you think about the latest commit.


差出人: Okadome Valencia @.> 送信日時: 2021年7月1日 20:46 宛先: arohl/gdis @.> CC: クレイグ・フィッシャー @.>; Author @.> 件名: Re: [arohl/gdis] Add option to scale balls according to vdW radii (#41)

PS: I forget to say : everything seems OK with your implementation but static analysis report the use of an undefined value in the file file_povray.c in the write_povray function, l. 425:

radius *= sysenv.render.ball_radius;

the static report is "The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage". Usually, setting an initial value (here for radius) at declaration is enough to get rid of such warning. Note that usually such warning are also not so important. ^^'

Best,

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/arohl/gdis/pull/41#issuecomment-872176776, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AP5N32DYZXSHOY2P7VUVLV3TVRISNANCNFSM47UAL6DA.

arohl commented 3 years ago

@cajfisher Can I ask what platform you are using to compile and run gdis?

ovhpa commented 3 years ago

@cajfisher everything seems fine with the new function for the display. However, it seems the povray images are stuck with a 0.5 vdW ratio. Is it the intended behavior?

cajfisher commented 3 years ago

I've changed the poray output to allow CPK scaling of b&s balls as well. Thanks for picking this up.


差出人: Okadome Valencia @.> 送信日時: 2021年7月5日 18:47 宛先: arohl/gdis @.> CC: クレイグ・フィッシャー @.>; Mention @.> 件名: Re: [arohl/gdis] Add option to scale balls according to vdW radii (#41)

@cajfisherhttps://github.com/cajfisher everything seems fine with the new function for the display. However, it seems the povray images are stuck with a 0.5 vdW ratio. Is it the intended behavior?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/arohl/gdis/pull/41#issuecomment-873972171, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AP5N32BCQ4BLZ6RBHKLEZYLTWF5TBANCNFSM47UAL6DA.

cajfisher commented 3 years ago

Removed all conflicts with master

ovhpa commented 3 years ago

@cajfisher Thank you for removing conflicts. I will check the PR tomorrow. In the meantime can you remove the file src/makefile as it is auto-generated by the install script? Also it seems that the file src/main.c is completely rewritten identically, is it another DOS end-of-line problem? Best,

cajfisher commented 3 years ago

Thanks. I've just made a few more corrections and improvements, so it should work properly now.

Yes, the src/main.c file was also in DOS format.


差出人: Okadome Valencia @.> 送信日時: 2021年7月8日 18:56 宛先: arohl/gdis @.> CC: クレイグ・フィッシャー @.>; Mention @.> 件名: Re: [arohl/gdis] Add option to scale balls according to vdW radii (#41)

@cajfisherhttps://github.com/cajfisher Thank you for removing conflicts. I will check the PR tomorrow. In the meantime can you remove the file src/makefile as it is auto-generated by the install script? Also it seems that the file src/main.c is completely rewritten identically, is it another DOS end-of-line problem? Best,

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/arohl/gdis/pull/41#issuecomment-876302385, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AP5N32HY7QXSVGZE7VQBV2LTWVY3DANCNFSM47UAL6DA.