search

phetsims / capacitor-lab-basics

"Capacitor Lab: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 4 forks source link

Stopwatch always in front even if voltmeter is grabbed #280

Closed brooklynlash closed 3 years ago

brooklynlash commented 3 years ago

Test device Lenovo ThinkPad

Operating System Windows10

Browser Chrome, Firefox

Problem description For https://github.com/phetsims/QA/issues/588 In the "Light" section, both the stopwatch and voltmeter can be grabbed, but even if you grab and drag around the voltmeter, it will not pop to the front.

Visuals capacitorstopwatch

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Capacitor Lab: Basics‬ URL: https://phet-dev.colorado.edu/html/capacitor-lab-basics/1.7.0-rc.8/phet/capacitor-lab-basics_all_phet.html Version: 1.7.0-rc.8 2020-12-17 01:25:13 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.141 Safari/537.36 Language: en-US Window: 1536x722 Pixel Ratio: 2.5/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 4095 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 32767x32767 OES_texture_float: true Dependencies JSON: {}

arouinfar commented 3 years ago

This looks a bit weird to me @Denz1994, but I think it's a relatively minor issue. If it's easy to do, it would be nice to bring the selected tool to the front. If the z-ordering is more complicated than that, we can close this as wontfix.

jonathanolson commented 3 years ago

Fixed in master with the above commit (can you verify?)

Also the rectangular hit-box for the voltmeter body seemed unfortunate (made the voltmeter a pain, as its hitbox extended well past its visual). I added pixel hit-testing for the body (which works nicely in master, so the hit area matches the visual)... however I used a Scenery flag that was implemented ~7 months ago.

As this unpublished RC version is using SHAs from... closer to 2 years ago, I'm curious about what common code it should be using (since that improvement will be ignored by the older SHAs).

Thoughts?

jonathanolson commented 3 years ago

Also, for this reason I did NOT push this to the 1.7 branch. Please explicitly let me know if I should do so (stacking will work, hit area will be weird).

arouinfar commented 3 years ago

@jonathanolson this is looking good in master.

As this unpublished RC version is using SHAs from... closer to 2 years ago

Yikes, I knew they were old, but I didn't realize quite how old. This redeploy has been back-burnered a few times because of competing priorities, and I really want to get it over the finish line. That said, it doesn't seem advisable to publish on such a stale branch. Taking new SHAs would require more QA time, but it would save @jonathanolson from cherry-picking the latest batch of sim changes.

My vote would be for taking new SHAs, but I can't make that call. @kathy-phet what would you advise?

kathy-phet commented 3 years ago

Yes, let's take new SHA's. Two years is really old ... yikes! @jonathanolson - Do we need to do a new dev on master, to test before moving it to a new RC with new SHAs?

jonathanolson commented 3 years ago

Do we need to do a new dev on master, to test before moving it to a new RC with new SHAs?

That would be my general preference, given the number of sim-specific code differences I saw between 1.7 and master. I'm also fine starting with an RC for 1.8 so that we don't need to redo any testing.

Preferences @kathy-phet or @arouinfar?

arouinfar commented 3 years ago

I think it makes sense to start with a dev test, but that means the QA tail will be longer. That's okay with me so long as we don't let things get too stale again.

kathy-phet commented 3 years ago

Let's start with dev test - once QA load lightens a bit - it will be good and dovetails with the idea that we will hopefully fit PhET-iO upgrade to new studio for this simulation later this year. So dev testing on the current master (phet brand) would be a good first step to be set up for that.

kathy-phet commented 3 years ago

@jonathanolson - can you please make an new QA issue, but hold off on creating building the dev version until we are ready to put it into the active test queue.

KatieWoe commented 3 years ago

This does look fixed in dev 5. Closing

brooklynlash commented 3 years ago

Reopening, because when you press the play or reset button for the stopwatch, it still stays behind the voltmeter. Kind of an inverted issue but I think it is similar enough. capacitance https://github.com/phetsims/QA/issues/616

jonathanolson commented 3 years ago

Due to the recent work in https://github.com/phetsims/scenery-phet/issues/659, this should be working better now in general. I tested master and it seems fine (moves to the front properly).

@brooklynlash can you test on master (phettest) to confirm?

brooklynlash commented 3 years ago

Looks fixed now! Thanks again. Re-closing.