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

Bond display and editing broken in version for issue #707 #710

Closed
MSoegtropIMC opened this issue Jul 30, 2021 · 14 comments
Closed

Bond display and editing broken in version for issue #707 #710

MSoegtropIMC opened this issue Jul 30, 2021 · 14 comments
Labels

Comments

@MSoegtropIMC
Copy link

Avogadro version: (please complete the following information from the About box):

Desktop version: (please complete the following information):

  • OS: Windows
  • Version 10 64 21H1

Describe the bug
When I open a cjson file in the #707 patch version, bonds are shown between atoms which are not bonded. Deleting bonds deletes arbitrary other bonds.

To Reproduce

  1. Load a .cjson file
  2. delete a bond in edit mode with right clock on bond
  3. a different bond then the one clicked on is deleted
  4. In addition in some cases bonds are shown between atoms which are not bonded

Expected behavior
Delete bond deletes the bond one clicks on.

Additional context
This did work in the previous patch release I have been using.

@ghutchis
Copy link
Member

Huh. It won't be from that patch, so if you can give me the previous release you were using, I can start with bisect to find the bug.

@ghutchis ghutchis added the bug label Jul 30, 2021
@MSoegtropIMC
Copy link
Author

The previous version (and the version I rolled back to for now) is the one linked in issue #680.

Can you reproduce the issue? I found that it does not happen for all files.

@ghutchis
Copy link
Member

ghutchis commented Aug 1, 2021

I've seen it a few times - I'm finishing the "Property Table" features and I've seen a few phantom bonds. I'm having trouble tracking it down, but it clearly requires deleting bonds.

Having the two versions I should be able to track down the patch. I have a few ideas, but it helps a lot, thanks.

@MSoegtropIMC
Copy link
Author

but it clearly requires deleting bonds.

I have one file where there was a phantom bond shown immediately after load (unfortunately I cannot share that specific file). But if it helps I can reinstall the version where this happens and double check.

@MSoegtropIMC
Copy link
Author

Is there anything I can do to help with this issue? I could e.g. try to create a file I can share which shows wrong bonds immediately after load if you think this might help.

@ghutchis
Copy link
Member

ghutchis commented Aug 6, 2021

Not sure if the file would help - I want to find the action that creates the invalid bond.

I've been on family vacation this week, but should be able to track it down on Sunday or Monday when I'm back.
(I don't want to release 1.95 until this is fixed...)

@MSoegtropIMC
Copy link
Author

What I wanted to say is that I think (need to double check - I currently don't have the version with this issue installed) I have a file which does show a wrong bond with this version but does not show the wrong bond with another version directly after loading the file. So there is not necessarily an explicit action which produces the wrong bond.

@ghutchis
Copy link
Member

ghutchis commented Aug 8, 2021

Thanks for the clarification. If you think that's the case, I'd be very interested in the file, yes.

@ghutchis
Copy link
Member

ghutchis commented Aug 9, 2021

Huh. I thought I was seeing your but, but I was seeing the new Cartoon rendering style (which somehow triggers on carbon atoms)
Screen Shot 2021-08-09 at 4 33 36 PM

Does your bug specifically require right-click to delete a bond?

@ghutchis
Copy link
Member

Can you take a look at the builds from #739
#739 (comment)

There were a few bond fixes in there - the unit tests had not been running properly on GitHub.

I'd like to know if I can close this or if you can help me reproduce it in a newer version.

Thanks!

@MSoegtropIMC
Copy link
Author

Hi Geoff, sorry I am busy these days and couldn't follow up on this. I will check the build you mentioned this weekend and in case it doesn't work see if I can create a simple example.

@ghutchis
Copy link
Member

Great, thanks. Hope we've managed to fix this for you.

@MSoegtropIMC
Copy link
Author

@ghutchis : I can confirm that the version you linked above does work. I have one file which does not load correctly (wrong bonds displayed immediately after load) in the version I mentioned in the initial post of this issue. The version you linked above from PR 739 displayes the same file correctly and I also couldn't find anything else odd in about 1 hour of testing. I am closing this issue.

P.S.: I love the new layer feature - don't know how I could live without it before.

@ghutchis
Copy link
Member

Great to hear, and glad you like the layer feature. The layer feature will get better. Working on ways to create a new layer from selection, make more commands layer-aware, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants