phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

vertexCutButton doesn't immediately update #980

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 13.2.1

Browser Chrome

Problem description For https://github.com/phetsims/qa/issues/908, If I click on the end of a circuit element so that the vertexCutButton appears and then attach it, the vertexCutButton still appears disabled. I need to click away and then click back on it for it to be enabled.

Steps to reproduce

  1. Add 2 circuit elements to the play area--ex. a wire and battery
  2. Click on one end of the battery
  3. Move the highlighted end of the battery and connect it to the wire

Visuals

https://user-images.githubusercontent.com/87318828/221943327-988f3918-546b-422d-a7d8-ad6380c40ddb.mp4

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Circuit Construction Kit: DC‬ URL: https://phet-dev.colorado.edu/html/circuit-construction-kit-dc/1.3.0-rc.1/phet/circuit-construction-kit-dc_all_phet.html Version: 1.3.0-rc.1 2023-02-23 22:57:25 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.3 Safari/605.1.15 Language: en-US Window: 1222x654 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
Nancy-Salpepi commented 1 year ago

In published, when I follow the above steps, the vertexCutButton disappears.

arouinfar commented 1 year ago

Good catch @Nancy-Salpepi! Perhaps this is an unintended consequence of https://github.com/phetsims/circuit-construction-kit-common/issues/975.

@samreid I think the behavior in published is preferable (dismissing the selection of the vertex). Here, the vertex being selected seems unintentional -- a user likely clicked the vertex before dragging it. However, if dismissing the selection is not compatible with the changes made in https://github.com/phetsims/circuit-construction-kit-common/issues/975, we should enable the cut button.

matthew-blackman commented 1 year ago

@arouinfar @samreid and I would like to try solving this by attaching the cut button visibility to the connected state of the vertex, as well as its selected state.

matthew-blackman commented 1 year ago

@arouinfar ready for your review

arouinfar commented 1 year ago

@matthew-blackman @samreid everything seemed fine the first few times connected a selected vertex to another vertex. However I ran into an assertion that froze the sim when making this connection:

image

image

Maybe the difference is that this vertex had three connected components?

samreid commented 1 year ago

Also this bug appears on CT.

matthew-blackman commented 1 year ago

@samreid it looks like https://github.com/phetsims/circuit-construction-kit-common/commit/477fa0f2b76cd2b171eddafb414c2f590b8f7019 has fixed the issue. I implemented the same pattern used in the updateStroke callback on VertexNode and confirmed that the circuit @arouinfar showed above no longer throws an error.

samreid commented 1 year ago

The fix looks appropriate, CT is clear and I ran a memory test and didn't see trouble. @arouinfar can you please re-test? If it seems OK please mark as ready for cherry-pick.

arouinfar commented 1 year ago

Looks great, thanks @samreid @matthew-blackman.

samreid commented 1 year ago

For the QA team, please test by selecting a vertex, then connecting it to another vertex and see if the cut button becomes enabled. Please try connecting a vertex to a lone vertex, and, in a subsequent test, to an already connected vertex.

Nancy-Salpepi commented 1 year ago

Looks good in rc.3! Closing.