1.46 to 1.51 upgrade hiccups

[eugene]

To put this post in a nutshell: migrating from version 1.46 to 1.51 has not gone smoothly for me.

I’m just going to make a numbered list, but (for me) the first one is by far the most important.

Also to anticipate a first suggestion/question: I have tried to use the tip revision, but it generated hundreds of errors in my code (even more than going from 1.46 to 1.51). I’m sure they are trivial to fix, but the amount of code I’d have to touch would make it a time consuming experiment. I say “experiment” because I wouldn’t feel comfortable doing a public release of our application using a beta version of Juce.

1. Font rendering has become blurry and grainy. I’m using Verdana in several sizes and weights, and the quality has degraded across the board. This problem has already been mentioned in the forum: http://www.rawmaterialsoftware.com/viewtopic.php?f=2&t=4746&start=0. Jules, I’m not sure what the current status of this is, but from my perspective the quality is such that I would have to keep using 1.46.

2. The opacity of Drawable objects gets ignored under certain conditions. I have a Drawable object, created from an SVG file, which I draw when the component’s paint function gets called. Something like this: drawble_obj->drawWithin( g, 0, 0, getWidth(), getHeight(), RectanglePlacement::centred, 0.3f ); The opacity is always treated as 1.0 (except if I pass in 0). In 1.51, drawWithin() boils down to a call to DrawableComposite::render().

void DrawableComposite::render (const Drawable::RenderingContext& context) const { if (drawables.size() > 0 && context.opacity > 0) { if (context.opacity >= 1.0f || drawables.size() == 1) { Drawable::RenderingContext contextCopy (context); …
I think the problem comes down to the ‘drawables.size() == 1’ check, which in my case is always true and the rendering is then always done with an opacity of 1.0.

3. It seems that the ‘const tchar* const’ form of functions has mostly been removed. The problem I ran into relates specifically to PropertySet::setValue (const String& keyName, const tchar* const value); however it could potentially exist in other places as well. For example, in 1.46 a call like setValue( keyName, T(“The Key Value”)) would be fine. In 1.51 (this is with VC 2008), it compiles without warning, but the T(“The Key Value”) argument is automatically converted to a bool and then uses the bool version of setValue(). Obviously not the desired result. One would expect the compiler to generate a warning here, but even at the strictest level it didn’t complain. I also tried turning off wchar_t as in internal type (/Zc:wchar_t-). This created dozens of linker errors for me though (and maybe correctly so).

Granted, this is not a problem with Juce, but it is an example of unexpected consequences related to an interface change. Jules, I’m curious as to why you changed it?

4. The UI/graphic performance on debug builds has become really slow. Is this expected, or could it be pointing to something I’m doing wrong with my app? I know it’s not really a big deal since the release performance is fine. It just makes debugging a bit tedious.

Apologies for the length of this post. Any suggestions or help is appreciated.

[godwincharan]

My suggestion would be to move one level up and try building the code… then one level up and so on…
so first convert 1.45 to 1.46…then 1.46 to 1.50… so on

[jules]

1. Font rendering has become blurry and grainy.

Font rendering on Windows (which I assume you’re talking about?) is done in exactly the same way, except that it now has greater horizontal accuracy, which makes it look better. I did tweak the vertical position slightly since the 1.51 release, so try the tip - it might just be that you were using a particular font/size combination that worked badly unless it was vertically aligned in a certain way.

I think the problem comes down to the ‘drawables.size() == 1’ check

No, that code is absolutely fine. If there’s a problem then it must be in the rendering of the child drawable, not the DrawableComposite. The drawable classes are going through a big rewrite at the moment anyway, so the code that I’m looking at probably bears little resemblance to the 1.51 release.

It seems that the ‘const tchar* const’ form of functions has mostly been removed.

I replaced them with Strings, so any existing code will work without modification, but it means that you can also just pass a const char* instead.

Your comment about PropertySet::setValue doesn’t make any sense - there’s simply no way that any compiler would silently convert a pointer to a bool, even if there was no other more suitable version of the method available (which there definitely is).

I can only think of one way that you could achieve the effect you’re describing, which would be if you’ve defined a global function that converts a const juce_wchar* to a bool - so maybe check that you’ve not accidentally got something like that hanging around in your code.

The UI/graphic performance on debug builds has become really slow.

I think I sorted that out a while ago in the tip.

[eugene]

Your comment about PropertySet::setValue doesn’t make any sense

I quite agree - and that was my own reaction when I saw what I described. However, you can verify this for yourself really quickly. Use the Juce Demo project from 1.51 and add the following code at the end of MainDemoWindow::MainDemoWindow().

ApplicationProperties::getInstance()->setStorageParameters( T("JuceDemo"), T("settings"), File::getCurrentWorkingDirectory().getFullPathName(), 1000, PropertiesFile::storeAsXML ); PropertiesFile *properties = ApplicationProperties::getInstance()->getUserSettings(); properties->setValue( T("TestKey"), T("Test Value") );
I’m using Visual C++ 2008. Notice that there are no warnings or errors when you build the project. Step into the setValue() call and see where you end up.

[jules]

Ok, tried it, and had a complete “WTF” moment. Yes, it does indeed silently cast a pointer to a bool. So does gcc, so it’s clearly the “correct” behaviour!

I thought I was getting pretty good at c++, but you learn something new every day! Here’s what it boils down to:

[code]class Test
{
public:
void foo (int x) {}
void foo (bool x) {}
void foo (const String& x) {}
};

Test t;
t.foo (“fgdfggd”); // calls the bool method!
[/code]

Without reading through all the small-print of the c++ standard, I assume that there must be a special case that allows casting of a pointer to a bool, so that people can write things like this:

[code]Thing* thing = getThing();

if (thing) // personally I always write “thing != 0” because I don’t like the implicit cast-to-bool, but most people don’t bother…
{
…etc[/code]

…so I guess that the compiler sees two possible casts it could choose from, and goes for the one that involves primitive types rather than creating a temporary object, despite it making no sense at all in this case.

All very annoying, but fascinating c++ minutiae, and I’ve learned a new pitfall to watch out for!

As for fixing it, what I’ll do is just replace all those setValue methods with a single one that takes a var object instead, and leave the var constructor to sort out the different types. Will check something in later today…

[otristan]

That’s why the explicit keyword is good for some constructor

[eugene]

If there’s a problem then it must be in the rendering of the child drawable, not the DrawableComposite.

Jules, if you’re in the process of a rewrite then this might be moot. But, my remark about ‘drawables.size() == 1’ being the problem was not stated correctly; or at least, I’ve figured out a bit more of the problem. The svg file I’m using to create the Drawable only has one sub-element (if that’s the correct phrase to use). In the Juce Demo, all the svg images have mutliple drawables within the parent. In the first call to render() the drawables.size() == 1; however, on the next call when it starts rendering the sub-elements (drawables.getUnchecked(i)->render (contextCopy)) the drawables.size() is larger than 1. In my case, drawables.size() always equals 1, even in the sub-elements. The net result is that it doesn’t make it into the else clause where it creates the composite layer and applies the opacity.

[code]void DrawableComposite::render (const Drawable::RenderingContext& context) const
{
if (drawables.size() > 0 && context.opacity > 0)
{
if (context.opacity >= 1.0f || drawables.size() == 1)
{
Drawable::RenderingContext contextCopy (context);

        for (int i = 0; i < drawables.size(); ++i)
        {
            const AffineTransform* const t = transforms.getUnchecked(i);
            contextCopy.transform = (t == 0) ? context.transform
                                             : t->followedBy (context.transform);

            drawables.getUnchecked(i)->render (contextCopy);
        }
    }
    else
    {
        // To correctly render a whole composite layer with an overall transparency,
        // we need to render everything opaquely into a temp buffer, then blend that
        // with the target opacity...
        const Rectangle<int> clipBounds (context.g.getClipBounds());
        Image tempImage (Image::ARGB, clipBounds.getWidth(), clipBounds.getHeight(), true);

        {
            Graphics tempG (tempImage);
            tempG.setOrigin (-clipBounds.getX(), -clipBounds.getY());
            Drawable::RenderingContext tempContext (tempG, context.transform, 1.0f);
            render (tempContext);
        }

        context.g.setOpacity (context.opacity);
        context.g.drawImageAt (&tempImage, clipBounds.getX(), clipBounds.getY());
    }
}

}
[/code]
To demonstrate the problem, insert the following svg image into the Juce demo. It will create an svg image with a single sub-element and the opacity slider will have no impact on how it’s rendered.

[code]<?xml version="1.0"?>

[/code] If you change it to this, the opacity starts working again (the number of sub-elements will be larger than 1).

[code]<?xml version="1.0"?>

[/code] Btw, my svg expertise is pretty much zero - I "borrowed" the original file from this site [url]http://www.croczilla.com/bits_and_pieces/svg/samples/[/url]

[jules]

The net result is that it doesn’t make it into the else clause where it creates the composite layer and applies the opacity.

Yes, I know, but like I said - that’s deliberate. If there’s 1 sub-element, then that element is expected to handle the job of dealing with the opacity. E.g. if you’re drawing one path object, it’d be stupid to render that path into a temporary image, then copy the image to the output with a new opacity, when you could just have let the path draw itself with a different opacity in the first place! It’s only when you have multiple objects in a layer that you need the temporary image.

There could indeed be a bug, but you’re looking in the wrong place - you’d need to look in the sub-element’s render method.

[eugene]

Maybe I’m missing something with how Drawable objects are used, so let me rephrase this from a higher level without trying to involve the internals of Juce.

I have a complex svg image (processed by binary builder) which I compile into my application. I create a Drawable object for the image by using Drawable::createFromImageDataStream(). When Component::paint() is called, I use Drawable::drawWithin() to render the image.

What doesn’t make sense to me is that the opacity passed to drawWithin() doesn’t have any effect on certain Drawables.

My svg image is not simple at all, but it only has one path element (it was created with Adobe Illustrator). For some reason, the opactiy I try to apply to the Drawable has no effect. In version 1.46 the opactiy was applied to the Graphics object passed into ::paint() and everything worked fine.

If I replace the svg image with one that has multiple sub-paths, the opactiy works as expected. However, this single/multiple sub-path distinction is somewhat beside the point. In use terms, I’m expecting a consistent behaviour for all svg images and I’m not seeing that. Should I be doing all this in some other way?

[jules]

Please stop repeatedly explaining the same thing! I understood you the first time! Like I said twice already, it sounds like one of the drawable classes (probably DrawablePath) is mistakenly ignoring the opacity value.

But I’ve rewritten all that code, and am still in the process of working on it, so I’m not going to start digging through old versions trying to find a bug that probably no longer exists. It you want a fix, try using the latest version, and if that still has a problem, I’ll fix it!

[eugene]

I’m not referring to an old version. The problem I described in my previous post happens in the tip revision.

[jules]

@eugene

I’m not referring to an old version. The problem I described in my previous post happens in the tip revision.

Well if you’d said so I’d have debugged it straight away!

Ok, it’s a pretty simple mistake - just needs to a couple of extra lines in DrawablePath. I’ll check something in soon, but here’s what you’d need to change if you’re in a hurry:

[code]void DrawablePath::render (const Drawable::RenderingContext& context) const
{
{
FillType f (mainFill);
if (f.isGradient())
f.gradient->multiplyOpacity (context.opacity);
else
f.setOpacity (f.getOpacity() * context.opacity);

    f.transform = f.transform.followedBy (context.transform);
    context.g.setFillType (f);
    context.g.fillPath (getPath(), context.transform);
}

if (isStrokeVisible())
{
    FillType f (strokeFill);
    if (f.isGradient())
        f.gradient->multiplyOpacity (context.opacity);
    else
        f.setOpacity (f.getOpacity() * context.opacity);

[/code]