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

Fix lotto positioning #124

Closed
2 tasks done
shippy opened this issue Jul 9, 2017 · 13 comments · Fixed by #132
Closed
2 tasks done

Fix lotto positioning #124

shippy opened this issue Jul 9, 2017 · 13 comments · Fixed by #132

Comments

@shippy
Copy link
Contributor

shippy commented Jul 9, 2017

The #115/#122 hotfix introduced some issues in lottery value display -- see clipping in SODM and especially HLFF. To close this issue, we need to:

@shippy shippy added the bug label Jul 9, 2017
@shippy shippy modified the milestones: Essential features, Essential bugfixes (v1.1.1) Jul 9, 2017
@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

The symptoms of the problem:

  • the top image slightly overlays the top label for monetary lotteries with images (-> not enough allowance for font size)
  • the top margin of the lottery box is larger than the bottom margin, meaning that the top text is farther away from the box than the bottom text
  • For some reason, this is by far the worst for the "nothing" display in HLFF high-fat condition when it is the top payoff/payoff associated with the blue. I should investigate why that is. (Moved to GStreamer text rendering on Windows breaks some text in HLFF/HF #131)

This problem is definitely introduced by the particular positioning that drawLotto is now using.

shippy added a commit that referenced this issue Jul 13, 2017
@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

Twist: I can't replicate the SODM bug on Windows, only the HLFF bug? (The screenshots were taken on a Mac)

@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

But the bug persists on Mac. Could this be related to #17? It's worth noting that on the Windows dev machine, I'm getting the following error:

PTB-DEBUG: DrawText: Failed to load external drawtext plugin [Unknown error].
PTB-INFO: DrawText: Failed to load external drawtext plugin 'libptbdrawtext_ftgl64.dll'. 
Reverting to legacy GDI text renderer. 'help DrawTextPlugin' for troubleshooting.

@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

Oh, goody. On a Mac, fixing this requires deleting a library from the Matlab installation. On a Windows machine, it requires installing GStreamer, which is likely to throw off other PTB installations – especially if they are relying on the native Windows rendering, which PTB considers incorrect. (And it can make them unstable.) Nonetheless, I will get GStreamer and see if that allows me to reproduce the issue in Windows.

@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

...yup, GStreamer installation broke the positioning in exactly the same way as it's broken on the Mac.

@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

On the one hand, using a platform-independent text rendering engine seems like a no-brainer.

On the other hand, almost all Levy Lab machines are running PTB on Windows without GStreamer, so this could break a lot of things.

A quick test suggests that if we take the GStreamer path (in getenv('GSTREAMER_1_0_ROOT_X86_64') || getenv('GSTREAMER_1_0_ROOT_X86') out of the system path after PsychStartup puts it there, it will fall back to the legacy renderer — and if we put it back in, it works. I guess that can be a switch in task options for people whose coordinates rely on the incorrect rendering?

@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

(Side note: since the screenshots for the tests were taken on Windows without the platform-independent rendering, installing GStreamer breaks those, too)

@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

So, for tasks that need the current positioning, there can be a settings option that calls breakGstreamer, which takes Gstreamer out of the system path:

function breakGstreamer()
%breakGstreamer Remove GStreamer, if available, from the system path
systempath = getenv('PATH');
systempath = erase(systempath, ...
  {[getenv('GSTREAMER_1_0_ROOT_X86_64'), 'bin'], ...
   [getenv('GSTREAMER_1_0_ROOT_X86'), 'bin']});
setenv('PATH', systempath);
end

In the meantime, I'm going to assume that everyone's running the complete PTB setup.

@shippy
Copy link
Contributor Author

shippy commented Jul 13, 2017

(...or maybe there's a way to hedge things and figure out how to correctly position elements for both options? I don't know. All I know is that this was supposed to be an easy fix and instead, I'm worrying about environmental variables.)

@shippy
Copy link
Contributor Author

shippy commented Jul 14, 2017

Steps:

  • fix the alignment problem under the generalized settings
  • see how much work it would be to fix the problem with breakGstreamer with the same configuration
  • see how much trouble it would be to keep 2 configurations, one for the general case and one for the Windows semi-setup (...I hear Draw scripts should take position as an argument #98 calling)

shippy added a commit that referenced this issue Jul 14, 2017
In my testing, this doesn't look bad on either Windows legacy renderer
or the platform-independent renderer. The positioning also makes sense.

Mostly, what fixed the positioning problemwas reverting #122.
@shippy
Copy link
Contributor Author

shippy commented Jul 14, 2017

It turns out that the fix for #115 (#122) actually inverted the correct placement of the payoff, and what was necessary was to invert everything else. Obviously, this will require further testing, but I think we've got it.

The positioning looks reasonably with and without GStreamer (tested by invoking breakGstreamer and re-running all the tests). I don't think I need to solve a problem that we don't have, so if anyone's setup depends on the Windows legacy renderer, we can do this then.

@shippy
Copy link
Contributor Author

shippy commented Jul 14, 2017

Remaining:

  • Create regression tests
  • Figure out, or create an issue for, and on the breakage of the cross-platform renderer for the particular case of the high-fat condition of HLFF

shippy added a commit that referenced this issue Jul 18, 2017
Tests the fix for regression in #124.

Generally streamlines the process of test creation - from now on, your
test should have the same name as the reference image. (cc: #63)
shippy added a commit that referenced this issue Jul 18, 2017
On a Windows machine with GStreamer installed, this can be tested by
invoking `breakGstreamer()` somewhere in the Matlab testing setup. See
issue #124 for reasons why anyone might want to do this.
@shippy
Copy link
Contributor Author

shippy commented Jul 18, 2017

With the creation of #131, all that's left to do is to merge this into master.

shippy added a commit that referenced this issue Jul 18, 2017
In my testing, this doesn't look bad on either Windows legacy renderer
or the platform-independent renderer. The positioning also makes sense.

Mostly, what fixed the positioning problemwas reverting #122.
shippy added a commit that referenced this issue Jul 18, 2017
Tests the fix for regression in #124.

Generally streamlines the process of test creation - from now on, your
test should have the same name as the reference image. (cc: #63)
shippy added a commit that referenced this issue Jul 18, 2017
On a Windows machine with GStreamer installed, this can be tested by
invoking `breakGstreamer()` somewhere in the Matlab testing setup. See
issue #124 for reasons why anyone might want to do this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant