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

Linux: Inactive buttons #544

Closed
tank-trax opened this issue Feb 8, 2019 · 10 comments
Closed

Linux: Inactive buttons #544

tank-trax opened this issue Feb 8, 2019 · 10 comments
Labels
Linux Issues which only occur on Linux

Comments

@tank-trax
Copy link
Collaborator

tank-trax commented Feb 8, 2019

the KEYTRACK and RETRIGGER buttons in the Oscillator section do not change colours, either to yellow or back to black, if clicked in Linux

they do appear to be active though

same for the red M and S buttons in the Scene section
the green Filter buttons just below however are functional

@tank-trax
Copy link
Collaborator Author

screenshot_20190208_173413

@baconpaul
Copy link
Collaborator

Thanks. Very specific and also very odd. Appreciate the detail.

@rghvdberg
Copy link
Contributor

confirmed here

@baconpaul baconpaul added the Linux Issues which only occur on Linux label Feb 10, 2019
@baconpaul
Copy link
Collaborator

baconpaul commented Feb 10, 2019

@rghvdberg a few controls (mute, solo, unipolar, relative, and link) use CSwitchControl rather than CHSwitchControl. The mute control is created in the ct_bool_solo case statement around line 856 of SurgeGUIEditor.coo

Oh and now reading the code, I know the bug.

CSwitchControl draws the background with a command that looks like pBackground->draw(...,0xff)

That 0xff sets the alpha to 255 which you would think is right. And is on mac. But there's a bug in cairo that the bitmap draw explicitly compares with 1.0 not 255. See the comment in line 164 of CScalableBitmap.cpp

So I think the fix for now is in CSwitchControl add

   float alpha = 0xff;
#if LINUX
  // FIXME: see comment in cscalablebitmap
   float alpha = 1.0
#endif

  ...
  pBackground->draw( ......, alpha )

and update CScalableBitmap comment only to point at the change we made here too.

Good luck!

@baconpaul
Copy link
Collaborator

oh and I haven't tested this or anything. This is just from looking at the code to write a guide on how it hangs together. So bug may be something else.

@tank-trax
Copy link
Collaborator Author

this may or may not be a clue

I ran Waveform9 and Reaper from the shell and when Surge loads in either DAW this error message appears...

scalable/bmp00110.png not found
scalable/[email protected] not found
scalable/[email protected] not found
scalable/[email protected] not found
scalable/[email protected] not found
scalable/bmp00150.png not found
scalable/[email protected] not found
scalable/[email protected] not found
scalable/[email protected] not found
scalable/[email protected] not found
Surge Error

could those be the missing images?

@rghvdberg
Copy link
Contributor

bmp00110.png = IDB_OSCSWITCH
bmp00150.png = IDB_BUTTON_CHECK
see resource.h
but I don't see them used in SurgeGUIEditor.cpp

@rghvdberg
Copy link
Contributor

I think that any bitmap drawn with CSwitchControl::draw() doesn't show up.

@rghvdberg
Copy link
Contributor

rghvdberg commented Feb 11, 2019

I've tried this CSwitchControl.cpp

   printf("get ViewSize() %f, %f\n",size.getHeight(),size.getWidth());
   auto pBackground = getBackground();
   auto testSize = pBackground->getSize();
   printf("pBackground size %f,%f\n",testSize.x,testSize.y);

output

get ViewSize() 15.000000, 22.000000
pBackground size 0.000000,0.000000

am I on to something ?

never mind :-)
false lead, in CHSwith2 the background gets 0,0 as size too

@baconpaul
Copy link
Collaborator

OK so crack debugging @rghvdberg the bitmaps have size 0x0 and have size 0x0 only on linux

The reason for this is because CScalableBitmap subclasses VSTGUI::CBitmap. On win and mac the parent constructor : CBitmap(desc) at line 41 or so sets up the parent class to load the original 100x asset; on linux it doesn't because resource resolution works differently on linux. As a result the parent class ::getSize() returns 0 on linux and returns the correct size on mac and win. Bummer

The reason anything works though is because CSwitchControl and only CSwitchControl reads the height of an image from the bitmap. Compare CHSwitch2::draw (which uses the member variable heightOfOneImage which is set on construction) to CSwitchControl (which uses pBackground->getHeight() if is_itype is false). And is_itype is always false.

OK so now we know the problem. We either need to

  1. Patch CSwitchControl so it doesn't get the height from pBackground->getHeight() or
  2. Patch CScalableBitmap so it responds to getHeight properly

The second seemed smart. Just override getHeight in CScalableBitmap and blammo fixxo. But getHeight is not virtual in cbitmap so nope. If we want to make CScableBitmap respond to get height we need to construct the base class properly.

So lets explore #1 a bit. The "is_itype" means it is a two state button and the top half of the image is off and the bottom half of the image is on. This matches what we see in the mute button image.

bmp00134 4x

So that translation is just moving half way down the image when it is on.

         CPoint where(0, (down ? (pBackground->getHeight() / 2) : 0));
         pBackground->draw(dc, size, where, 0xff);

But the image takes up the entire rect into which it draws, so our constructor knows how big it is. This leads me to the easiest approach

  1. Add a member 'heightOSinglefImage' to CSwitchControl.cpp
  2. In the constructor set heightOfSingleImage = size.getHeight()
  3. In the draw method replace pBackground->getHeight() with heightOfSIngleImage
  4. Test on linux; if it works make a PR and we will test on mac and win and then merge

The other path is harder, which is make CScalableBitmap pass a proper thing to its base class. To do that

  1. Factor the code which makes a filename at line 80 into a member function of CScalableBitmap
  2. Replace the scamp #if MAC with that function rather than filename.str().c_str()
  3. Make the try block in an #if linux into another function which is #if linux, called perhaps "platformBitmapForIDAndScale" which calls the filename function then does that try block. Call it around line 83
  4. Once that function is written change the constructor around line 41 so
   : CBitmap(desc)

becomes

#if LINUX
   : CBitmap( makePlatformBitmapForIDAndScale( desc.u.id, 100 ) )
#else
  : CBitmap( desc )
#endif`

and test and sweep. That is in some sense "more correct" but it is also a way bigger change. I would approve the first PR with the simpler approach in a heartbeat if it works.

Hope that helps!

baconpaul pushed a commit to baconpaul/surge that referenced this issue Jul 10, 2019
…#588)

Solo, Mute, and other CSwitchControl buttons were inactive on Linux because CScalableBitmap returns size 0,0 for all bitmaps on Linux. This diff doesn't solve that problem but accommodates it by making CSwitchControl work like the other switch controls; using a constructor specified size (in this case the size of the view rectangle) to determine how much to translate the switching image. This makes solo, mute, and others work on Linux.

Closes surge-synthesizer#544 by making those buttons inactive.

Former-commit-id: a2a858c26719c32a55904661ee9b7ca12eefdc66 [formerly 40e18d1]
Former-commit-id: 40ef5151482e5843a0503ce5d2fb87d0a0d19eaa
Former-commit-id: df1b0bc9beb018eb159d39e1000c085d465c2378
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linux Issues which only occur on Linux
Projects
None yet
Development

No branches or pull requests

3 participants