search

phetsims / build-a-molecule

"Build a Molecule" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
8 stars 7 forks source link

unable to create 4-carbon chain #148

Closed arouinfar closed 4 years ago

arouinfar commented 4 years ago

Noticed during #131

It should be possible to connect four carbons in a straight chain, but that's not currently possible. 4-c

However, it is possible to connect them in this arrangement image

Denz1994 commented 4 years ago

This issue is due to structuresData.js not containing the valid structure for C4 as-built above.

More details regarding implementation:

The allowed structures are validated every time and atom is attempted to bond (see Kit.attemptToBondMolecule). From this, the attempted bond is validated against the master list structuresData.js of allowed structures via a hash string. If the structure that is built from the molecule bonds (StrippedMolecule) doesn't exist in that master list, then it is rejected.

The workaround is to amend the structuresData.js with the entry for C4 as it is built and bonded above. @jonathanolson didn't see why this structure shouldn't exist and perhaps this is related to the need to regenerate new data for the structures (similar to #158). There is a sanity check to prevent looping but that didn't seem promising when removing the check (see MoleculeStructure.hasLoopsOrIsDisconnected)

@arouinfar Can you confirm that C4 can be created in master?

Denz1994 commented 4 years ago

Note: The above measures to prevent looping were implemented to prevent structures like this.

image

arouinfar commented 4 years ago

@Denz1994 something's still not quite right. I can connect 4 carbons in a straight line, but I'm then unable to attach any hydrogens to the carbon chain UNLESS I first attach something else like an N or O.

C4

I'm also finding cases like this that I can make in Java, but not in HTML5:

However, the data for this molecule can be found in OtherMoleculesData.js acetic acid ethyl ester|C4H8O2|8857

Denz1994 commented 4 years ago

The structure data has been inputted for the above-mentioned molecules. Can you confirm this is the case on Master @arouinfar?

arouinfar commented 4 years ago

Things are generally working now @Denz1994. I'm seeing some buggy behavior that I'm not 100% sure how to consistently reproduce. Occasionally, I am unable to create large molecules, and it seems like it happens if I've used the button to cut all bonds

image

Can you play around with it a bit and see if you can figure out what's going on? I also recommend having QA bang away on this issue during the next dev test.

arouinfar commented 4 years ago

@Denz1994 something strange is going on. I'm currently trying to verify #169, and was trying to build acetic ethyl acid ester from https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-585432259. I was able to build it earlier today on master (see the screenshot in #169), but now I don't seem to be able to.

My current procedure is:

  1. Open Playground and scroll to 2nd page of carousel.
  2. Make C-C-O-C by dragging atoms out of the bucket and immediately connecting them to the chain.
  3. Try to connect a 4th carbon to the end of the chain to make C-C-O-C-C but it bumps away like it's an illegal structure.

EDIT: I was able to get around this by adding the 2nd oxygen before trying to connect the 4th carbon. The order in which atoms are connected should not matter.

Alternative step 3:

Adding 4th carbon is now possible:

Denz1994 commented 4 years ago

Regarding https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-606233607:

A testing cycle may look something like this:

  1. Build a specific molecule from a set of large molecules, while tracking the order by which the molecule was built.
  2. Break the molecule using the blue arrowed button
  3. Reconstruct the molecule from step 1, using a different build order.
  4. Repeat 1-3 for the remaining set of large molecules.

*The point is the order of the build path shouldn't matter to construct the molecule

I've tried these steps with the below molecules and can't replicate the concerns brought up https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-606233607. @arouinfar Should we add more molecules to test? Also, what do you think about the above-proposed testing cycle?

Here are the molecules I tested with: image

image

image

image

Denz1994 commented 4 years ago

@arouinfar Could you try this dev version for reviewing https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-606283587. The build order for acetic ethyl acid ester shouldn't matter in this build.

Denz1994 commented 4 years ago

@arouinfar and I revisited the issue mentioned in https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-606233607. I wasn't able to reproduce locally, and @arouinfar thought this may be related to chrome caching an older version of BAM. We thought a QA member could take a look at this.

@KatieWoe Summary: The order by which a molecule is built shouldn't matter. It was noted that occasionally building a molecule in a specific order fails. I've given some instructions on https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-611099900 to help test this issue. Could you try these steps on a few large molecules and determine if the build order affects if they can be built or not? I've listed some molecules that may be a good start above.

KatieWoe commented 4 years ago

You can create https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-584965693 in the above dev version @Denz1994

Denz1994 commented 4 years ago

Were you able to replicate https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-611099900 @KatieWoe?

KatieWoe commented 4 years ago

No, but I haven't been able to dedicate a lot of testing time to it due to other tests.

arouinfar commented 4 years ago

@Denz1994 @KatieWoe I am unable to reproduce the exact problem described in https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-606283587, but I'm running into a related problem. It seems like the issue is localized to molecules containing chains like C-C-O-C-C or C-O-C-C-C. After building these chains, I am unable to attach any hydrogens to the carbons, despite there being several structures in the database that contain these chains.

Procedure:

  1. Playground screen, Kit 2
  2. Create C-C-O-C-C, adding atoms in order from left-to-right.
  3. Try to add a hydrogen to a carbon. Regardless of the carbon, it will be repulsed.
  4. Add an oxygen to the 2nd or 3rd carbon. For example: image
  5. Carbon-hydrogen bonding is now possible.
Denz1994 commented 4 years ago

I misinterpreted the check that prevents https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-584965693. This was allowed in the legacy version as well.

The concern was regarding a user no knowing what atoms are bonded in the example in https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-584965693. However, with the addition of the cut targets (yellow circles), it clearly shows what atoms are bonded. I don't think this is an issue. Tagging @arouinfar to confirm.

Denz1994 commented 4 years ago

Also, the above commits should have fixed the order dependent bonding problem that was mentioned by @arouinfar above. Can you test in this dev version @arouinfar?

@jonathanolson and I did some intense deep-diving into the visitor arrays in checkEquivalency(). It looks like I was removing the incorrect items from the visited atoms causing these false equivalencies.

Dev version: https://phet-dev.colorado.edu/html/build-a-molecule/1.0.0-dev.74/phet/build-a-molecule_en_phet.html

arouinfar commented 4 years ago

This appears to be fixed in dev.74 @Denz1994.

If the sim is in a good place, it would be nice to publish a new prototype to the website.

Denz1994 commented 4 years ago

This issue has been listed for QA to test with specific instructions in https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-611099900.

KatieWoe commented 4 years ago

I noticed (using https://github.com/phetsims/QA/issues/506) that, if the molecule is built in a certain way, there may not be room to add some of the atoms needed. Is this acceptable? Trying to build this in a slightly different configuration: cantfinishthis No room to add last atoms: notfinished

Denz1994 commented 4 years ago

Correct me if I'm wrong @arouinfar, but I think the second image in https://github.com/phetsims/build-a-molecule/issues/148#issuecomment-641437201 is technically a different molecule. For instance, the center carbon doesn't have two fluorines bonded to it.

If that is the case then this is acceptable.

KatieWoe commented 4 years ago

It is a different molecule, but I was trying to build the top one. That second Fluorine can't be added because there is no room for it, it just bounces off. That is what I was trying to report.

Denz1994 commented 4 years ago

Ah, I see now. I think this is more in line with the atom layering issue. I don't think this is necessarily buggy behavior anymore because the user picked a path to build the molecule in a way that makes them run out of space. This is different than having the space to build and the correct atoms and it failing.

I'll leave to @arouinfar for input.

arouinfar commented 4 years ago

I don't think this is necessarily buggy behavior anymore because the user picked a path to build the molecule in a way that makes them run out of space. This is different than having the space to build and the correct atoms and it failing.

Agreed @Denz1994. I think it's important for students to thoughtfully place their atoms when building large molecules. I don't see the current behavior as a bug, but rather a productive constraint.