attempt replace GodleyIcon::iconScale() by Item::scaleFactor() toward… by wdednam · Pull Request #194 · highperformancecoder/minsky

wdednam · 2020-07-21T20:59:44Z

…s fixing tickets 1209 and 1213. I don't understand why Item::scaleFactor behaves differently to GodleyIcon::iconScale if I set it the same way in GodleyIcon::scaleIconForHeight?

This change is

highperformancecoder · 2020-07-21T22:10:46Z

+                // ensure that flows used as intial conditions, which inherit their values from other vars, correctly initialise corresponding stock var. for ticket 1137.
+                //auto vv=minsky().variableValues[VariableValue::valueIdFromScope(group.lock(),v.init)];
+                //if (vv->lhs()) vv->init=str(vv->value());                


I've removed it, thanks.

highperformancecoder · 2020-07-21T22:11:06Z

-                FlowCoef fc(v.init);
-                table.cell(r,c)=fc.str();
+                FlowCoef fc(v.init);   
+				table.cell(r,c)=fc.str();


funny indenting

Fixed this up in Emacs.

highperformancecoder · 2020-07-21T22:11:29Z

  {
    double dx=fabs(x-this->x()), dy=fabs(y-this->y());
    auto z=zoomFactor();
+    //double w=0.5*Item::width()*z, h=0.5*Item::height()*z;


highperformancecoder · 2020-07-21T22:11:44Z

    float flowMargin=0, stockMargin=0;
    /// icon scale is adjusted when Godley icon is resized
-    float m_iconScale=1;
+    //float m_iconScale=1;


highperformancecoder · 2020-07-21T22:12:27Z


    /// scale icon until it's height matches \a h 
-    void scaleIconForHeight(float h) {update(); m_iconScale*=h/(bottomMargin()+iHeight()*iconScale()*zoomFactor());}        
+    //void scaleIconForHeight(float h) {update(); m_iconScale*=h/(bottomMargin()+iHeight()*iconScale()*zoomFactor());}        


highperformancecoder · 2020-07-21T22:12:53Z


    /// icon scale is adjusted when Godley icon is resized
-    float iconScale() const {return m_iconScale;}
+    //float iconScale() const {return m_iconScale;}


highperformancecoder · 2020-07-21T22:16:38Z

All looks reasonable, minor nits aside. Just waiting to see that it passes Travis.

wdednam · 2020-07-22T12:13:28Z

Thanks. I've committed a better version now, but there are still some issues, mentioned by Steve in tickets 1209, 1213 and now 1214.

highperformancecoder · 2020-07-23T02:26:31Z


    /// icon scale is adjusted when Godley icon is resized
-    //float iconScale() const {return m_iconScale;}
+    float scaleFactor() const override;


This is identical to Item::scaleFactor(), so not needed.

Removed it.

highperformancecoder · 2020-07-23T02:27:03Z

    /// icon scale is adjusted when Godley icon is resized
-    //float iconScale() const {return m_iconScale;}
+    float scaleFactor() const override;
+    float scaleFactor(const float& sf) override;


I'm assuming this override is to prevent updating the boundingBox, is that correct?

Yes, because the bounding box was wrong after some resize operations.

highperformancecoder · 2020-07-23T02:28:29Z

    float flowMargin=0, stockMargin=0;
    /// icon scale is adjusted when Godley icon is resized
-    //float m_iconScale=1;
+    float m_sf=1;


Why not use Item::m_sf here?

I guess I can dynamic_cast to Godley in Item::scaleFactor(const float& sf) to prevent the bounding box from being updated in that case.

highperformancecoder · 2020-07-23T07:58:03Z

On Thu, Jul 23, 2020 at 12:44:35AM -0700, Wynand wrote: @wdednam commented on this pull request. ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ In model/godleyIcon.h: > @@ -40,7 +40,7 @@ namespace minsky /// for placement of bank icon within complex float flowMargin=0, stockMargin=0; /// icon scale is adjusted when Godley icon is resized - //float m_iconScale=1; + float m_sf=1; I guess I can dynamic_cast to Godley in Item::scaleFactor(const float& sf) to prevent the bounding box from being updated in that case.

No need. Just make m_sf protected, not private, then update in the overridden scaleFactor(const float& sf)

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.*

--

---------------------------------------------------------------------------- Dr Russell Standish Phone 0425 253119 (mobile) Principal, High Performance Coders hpcoder@hpcoders.com.au http://www.hpcoders.com.au ----------------------------------------------------------------------------

wdednam · 2020-07-23T08:49:19Z

No need. Just make m_sf protected, not private, then update in the overridden scaleFactor(const float& sf)

Mmmm, I've tried the above, but when I use Item::zoomFactor() in the GodleyIcon Class, I also have to use it in the VariableBase class where attached variables' zoomFactor are calculated and then the Godleyicon does not resize properly...

highperformancecoder · 2020-07-23T22:36:39Z

+      {	
+        if (y.width) x1->iWidth(*y.width);
+        if (y.height) x1->iHeight(*y.height);
+        x1->bb.update(*x1);  		    


As per comment on PR#195, this update call should be redundant.

highperformancecoder · 2020-07-23T22:39:34Z

+  protected:
+     float m_sf=1;


I meant use Item::m_sf. I thought maybe you didn't have access, because Item::m_sf might have been private, hence the comment about making it protected, but I see it is actually public right now.

"Overriding" a member variable like this is going to cause a lot of confusion going forward.

… of Minsky

wdednam · 2020-07-27T18:18:40Z

This is my best attempt so far. The test suite passes locally. i hope it also does remotely.

wdednam · 2020-07-27T18:21:24Z

        accumulateWidthHeight(m_stockVars, stockH, stockMargin);
        accumulateWidthHeight(m_flowVars, flowH, flowMargin);
-        float iw=this->iWidth(), ih=this->iHeight();
+        float iw=this->iWidth()*this->zoomFactor(), ih=this->iHeight()*this->zoomFactor();


I'm comparing with the sum of all v->width() or all v->height() with g->iWidth() or g->iHeight(). I added the zoomFactor in item::width() and item::height() for ticket 1204, so I need to multiply iWdith() and iHeight() by the zoomFactor here

This was a mistake, The fact that v->width() and height() now contain the zoomFactor causes problems, as shown in ticket 1221. I'm reverting it to RenderVariable width() and height() and removing the zoomFactors here.

highperformancecoder · 2020-07-27T22:10:44Z

    static void setPrivates
    (minsky::GodleyIcon& g, const vector<vector<string> >& data, 
-     const vector<GodleyTable::AssetClass>& assetClass,float iconScale)
+     const vector<GodleyTable::AssetClass>& assetClass)
    {
      setPrivates(g.table, data, assetClass);
-      g.m_iconScale=iconScale;
    }


We can now get rid if this helper function too.

I just tried this and it caused the variables attached to GodleyIcons to disappear. So, I've left it in.

highperformancecoder · 2020-07-27T22:43:43Z

I took this for a spin, and it is looking good. The resulting shape of the icon still seems a little counterintuitive when shrinking the icon down towards the flow and stock variables (the icon should really maintain its aspect ratio with the scaleFactor adjust the variables to fit, IMHO). I did make one change setting iWidth/iHeight had a mysterious factor of 1/2, which made no sense. Removing that improved things a little bit. I have integrated the changes now.

I also checked the status of old schema - nothing too hilarious there!

highperformancecoder reviewed Jul 21, 2020

View reviewed changes

wdednam marked this pull request as ready for review July 22, 2020 12:09

highperformancecoder reviewed Jul 23, 2020

View reviewed changes

wdednam added 2 commits July 27, 2020 11:54

another update that does not work

13b7cc6

best fix I could come up with. iconScale has been wiped from the face…

3ca5620

… of Minsky

wdednam force-pushed the noIconScale branch from 24170e8 to 3ca5620 Compare July 27, 2020 18:17

wdednam commented Jul 27, 2020

View reviewed changes

highperformancecoder reviewed Jul 27, 2020

View reviewed changes

highperformancecoder merged commit 3ca5620 into highperformancecoder:master Jul 27, 2020

highperformancecoder added a commit that referenced this pull request Dec 17, 2021

Performance optimisation. For varun/minsky #194

12c629f

highperformancecoder mentioned this pull request May 29, 2026

feat: move display Godley table variable values as an attribute of Go… #662

Merged

Uh oh!

Conversation

wdednam commented Jul 21, 2020 • edited by highperformancecoder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highperformancecoder commented Jul 21, 2020

Uh oh!

wdednam commented Jul 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highperformancecoder commented Jul 23, 2020 via email

Uh oh!

wdednam commented Jul 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdednam commented Jul 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highperformancecoder commented Jul 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wdednam commented Jul 21, 2020 •

edited by highperformancecoder

Loading