Open samreid opened 7 years ago
I'm not seeing 1.2.5 mentioned in this repo or tasks repo issues. But its shas do differ from 1.2.3. I guess I should use 1.2.5 shas?
My apologies! I was out of date in the branch. It looks like this bumped to 1.2.5 in a maintenance release with this commit message: Bumping version to 1.2.5 for phetsims/phet-android-app#16,https://github.com/phetsims/phetcommon/issues/37,https://github.com/phetsims/vibe/issues/30,https://github.com/phetsims/tasks/issues/818
The shas to cherry pick are: https://github.com/phetsims/chipper/commit/1cb1805c9c05467f3ea2cbe1657b2f2b3deb0e8a
and https://github.com/phetsims/joist/commit/bb8957a9a6395aed86750051afc69521d6075def
So I guess I'll need branches for these.
It looks like Build an Atom 1.2.5 chipper predates query string machine, so that won't work as a cherry-pick. Build an atom 1.2.5 joist is not using Emitters, so this cherry pick won't work either.
Main choices from here: (1) build custom "legacy" patches for this sim and possibly others (2) fully retest build an atom
Tagging @ariel-phet and @kathy-phet so they are aware what's happening here.
I pushed a custom patch in the above commit. @jessegreenberg can you please review the change?
I made this change in the maintenance branch, but not sure if that's the best spot--I'm concerned this would leak into production if another maintenance release is done.
I published an RC here: http://www.colorado.edu/physics/phet/dev/html/build-an-atom/1.2.6-rc.1/build-an-atom_en.html?legendsOfLearning
It tests OK in the harness in Chrome.
@kathy-phet and @ariel-phet it would be good to get your feedback for next steps. Some possibilities:
@samreid I am not concerned about "RC" showing up in the about dialog, since almost no user will look there, and even fewer will know what RC means
However, if we are going to be regularly publishing to legends of learning, this code should be in production and well tested (since maintenance releases are sure to occur, as they happen regularly). If we are going to make this code standard it should have QA done when put in production.
If this is just for a prototype and needs quick attention, the QA team is currently back logged (which should be improved in the next week or so as team members start summer schedule.
Would it be OK to deliver the prototype, make sure things are working on LoL end, make any necessary tweaks, and then do a full QA test to bring to production?
@ariel-phet @kathy-phet and I should discuss this on Thursday
Also @jbphet addressed #161 so if we cherry-pick that fix we could get the timer to pause on the game screen. Not sure if that is important for first prototype in their platform.
@jessegreenberg and I tried to cherry pick the change but the branches diverged too much so it couldn't auto-cherry-pick. We ended up applying the same changes manually. It seemed OK in testing.
New RC is here: http://www.colorado.edu/physics/phet/dev/html/build-an-atom/1.2.6-rc.2/build-an-atom_en.html?legendsOfLearning
I'll create a tasks issue for testing it.
One problem was discovered during RC testing: https://github.com/phetsims/scenery/issues/619
@jonathanolson and I solved it by adding interrupt methods to input listeners. However, neither of the commits is cherry-pickable for the build an atom SHAs.
@jessegreenberg are you available to create a new Build an Atom RC that uses the same SHAs as http://www.colorado.edu/physics/phet/dev/html/build-an-atom/1.2.6-rc.2/build-an-atom_en.html?legendsOfLearning with the two new commits (probably manually patched in) from scenery and joist in https://github.com/phetsims/scenery/issues/619 ?
@samreid sure, I will prepare the next RC today.
Thanks and good luck! Let me know how tricky it is to patch in those commits, we may be doing it for 11 more legacy joist and scenery shas.
Merging was pretty easy, all things considered. There were merge conflicts in Sim.js and SimpleDragHandler.js, but they were not too difficult to work through (things like renaming to self
and such). Node.js was missing implementations for interruptInput and interruptSubtreeInput, I had to include those, but that was straight forward.
However, I am still seeing a bug in the legends of learning test harness, particles freeze after pressing pause during dragging
To see this in the harness,
Particles will continue to move until they hit this position. This is a problem for both master and 1.2 branches. Does not happen every time.
Not sure if this is a sim specific bug or a problem with changes in https://github.com/phetsims/scenery/issues/619. This never happens when changing screens, only when using Legends of Learning pause feature.
In the image https://github.com/phetsims/build-an-atom/issues/162#issuecomment-308222785, pressing pause and then resume again makes the particles return to their correct places.
How old are the SHAs that need to be patched for this? It may require other Scenery changes if it's been any appreciable amount of time.
It looks like 1.2.0 was build on April 22, 2016. But the bug described in https://github.com/phetsims/build-an-atom/issues/162#issuecomment-308222785 is happening for master as well.
I tested master with 1 particle, 2 particles and 3 particles (testing once each) on iPad3 and didn't see the problem in https://github.com/phetsims/build-an-atom/issues/162#issuecomment-308222785
I'll try the branches.
After checking out build-an-atom 1.2 and running grunt checkout-shas I cannot get the requirejs version of the sim to run on the harness on the iPad3. Runs fine on the desktop, but I cannot test the touch problem. I also did not see a published RC--is that correct?
Tethered I saw problems like these:
Perhaps I need an older chipper?
Killing iPad3 safari and changing the question seems to have helped. Perhaps it was caching something bad.
Testing on the branches on iPad3 as described in https://github.com/phetsims/build-an-atom/issues/162#issuecomment-308222785
About half the time either the proton or neutron or both get stuck in the air. But they seem draggable after reset all (that is different than when we first began). May be time to get @jbphet + @jonathanolson help on this?
Properly cherry-picking commits from Scenery to a branch 300+ commits old (and whatever Joist is) sounds risky.
It looks like things like the ES5 getter for inputListeners was handled, but I haven't exhaustively checked for where things are going wrong.
Also https://github.com/phetsims/scenery/commit/36aa205e3f85c86aa3ced7c813133c54611ddc73 has a typo checking hanlder.dragging
.
Porting to each Scenery/Joist combination would probably require a lot of manual checks for the existence of all the APIs being used, in addition to QA testing (as many things are easy to break, and the workarounds in master aren't guaranteed to work for older sim code).
Since it would potentially use QA resources anyways for testing, deploying out of master seems like the superior option?
Tagging @ariel-phet and @kathy-phet so they are aware of the issue and the possibility of deploying out of master instead of maintenance branches.
Also tagged for developer meeting in case we forget to talk about this at status meeting.
Sam, I thought I saw that Jesse had trouble with this bug still being present even out of master?
I tend to agree with JO here. Would this fix pick up the input focus issues in general that jo has been improving?
It would be nice to test this one reasonably soon, as other priorities wane. Kathy
Sent from my iPhone
On Jun 14, 2017, at 8:20 AM, Sam Reid notifications@github.com<mailto:notifications@github.com> wrote:
Also tagged for developer meeting in case we forget to talk about this at status meeting.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/phetsims/build-an-atom/issues/162#issuecomment-308446152, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AE3FZOe7qtgae4mPfHrwZw9c924YbmIcks5sD-wPgaJpZM4NoiYS.
I also did not see a published RC--is that correct?
Right, I noticed the problem before building an RC.
Also phetsims/scenery@36aa205 has a typo checking hanlder.dragging.
Indeed, sorry, but was noticed and fixed in https://github.com/phetsims/scenery/commit/f9dc3cc5ee44a6ee7ec78d09a40a7d3577e9620e before this checking.
I thought I saw that Jesse had trouble with this bug still being present even out of master?
Yes, this is happening in Windows 10 Chrome out of both master and the release branch.
I can hop in if needed, but if deploying from master fixes the issue(s), then +1 on that thought.
@samreid - For the purposes of testing the LoL approach, if it is simpler to pick one of the other sims that LoL identified as a match that is also built on more recent Shas -- e.g. Unit Rates -- that could be a possibility for an alternative basic test with LoL while this particular bug is worked out for BAA.
I'll be available soon to investigate what is still happening in master.
I tested master on iPad3 5 times and saw the problem 0/5 times. Is it possible that @jessegreenberg master tests had a cached version? Sometimes iframes cache themselves more eagerly than top-level pages.
I don't think so, tested again in a private browsing window, and the following image shows a "stuck" neutron above the about dialog with 1.3.0-dev.1 as the version, dependencies have master checked out.
This is starting to feel more like a sim bug, maybe something isn't getting reset when the input is interrupted. I hit the problem again by
But this isn't happening very consistently, maybe 1/8 times or so.
Running through the steps in https://github.com/phetsims/build-an-atom/issues/162#issuecomment-308506610, observed the behavior 2/5 times.
Is it possible to add something like a console.log to inside SimpleDragHandler's interrupt (inside the dragging check), to see if endDrag is getting called? I'm curious whether the buggy behavior is preventing that from getting interrupted, or if it's after the interruption happens.
Yes, I added a console statement on line 192 of SimpleDragHandler, it is well after the interruption happens. I hit the bug on my second attempt by following these steps:
Ah, if I press "Pause" after the steps in https://github.com/phetsims/build-an-atom/issues/162#issuecomment-308585727, I see "interrupted" again in the console, and then the neutron returns to the bucket after pressing resume.
I tried several more times but am still unable to reproduce this problem on master. I'm on iPad3.
Following the steps in https://github.com/phetsims/build-an-atom/issues/162#issuecomment-308585727, I was able to see this 4/10 times on master on an iPad2 air (PhET device Pauling). Sometimes you have to drag it more than twice after resuming for it to get stuck.
@jonathanolson and @jbphet will look into this by tomorrow, possibly with help from @jessegreenberg
Properly cherry-picking commits from Scenery to a branch 300+ commits old (and whatever Joist is) sounds risky.
It sounds like we all agree that maintenance releases and patching is too risky for these releases because the SHAs are too old.
@samreid This issue is labeled for developer meeting. Can you summarize what feedback you're looking for?
From the last meeting:
@jonathanolson and @jbphet will look into this by tomorrow, possibly with help from @jessegreenberg
It would be good to get a status update whether in issue comments or in developer meeting. I'll leave it marked for developer meeting in case it doesn't get discussed elsewhere.
On 6/15/20, @jbphet, @jonathanolson and I followed the steps listed in https://github.com/phetsims/build-an-atom/issues/162#issuecomment-308585727 on @jbphet's machine on master and observed the particles getting stuck after a couple attempts.
And there are some updates in the investigation of https://github.com/phetsims/scenery/issues/619
It sounds like @jonathanolson is confident in the fix for #phetsims/scenery#619. I will create an issue in QA to verify that things are looking good before proceeding with the next RC.
If I'm reading it correctly, the solution is working well on all platforms except Edge, which we decided is OK for now. So it seems the next step is to publish a Legends of Learning prototype--will this need another QA cycle? Or is there some safe SHA we should start with?
As part of https://github.com/phetsims/joist/issues/423 I am building a Legends of Learning prototype of Build an Atom. I want to use safe SHAs to limit the amount of necessary testing for the QA step. The latest published version is 1.2.5. However, the 1.2 branch dependencies.json says 1.2.3. @jbphet can you please help me understand this discrepancy?