Automatically set LinePattern.None instead of solid lines for gradients #4711
Conversation
|
Note that some of them already had this. |
maltelenz
left a comment
maltelenz
left a comment
There was a problem hiding this comment.
Why? The link does not explain why this change would be something you want.
Why wouldn't you be allowed to have a line around a shape with a gradient?
You are allowed to have them, but often they don't make sense - so I removed for all that didn't specify pattern and had gradient. In many cases it doesn't matter, since there's something else drawing something on top of the border. So to be concrete:
However, it's a draft PR as I haven't looked at all of them yet. |
|
I'll gladly review the effects in System Modeler when you consider it ready! |
|
Note: Will need to make a separate PR for |
beutlich
changed the title
|
Partial verified list of changes: Duplicated lines, which makes it obvious that border for the gradient-shape should be skipped:
Others that I think are improvements without border:
Will skip after review - the change wasn't "wrong" but there were other issues:
Skipping spherical for ellipses (and a few rectangles), since it is mostly redundant and tools that don't have gradient at line-color at the circle radius will benefit:
|
Modelica/Magnetic/QuasiStatic/FundamentalWave/Losses/PermanentMagnetLosses.mo
Outdated
Show resolved
Hide resolved
Polygons that are not exactly level shouldn't use cylinder-gradient.
|
For Modelica.Mechanics. I note that WSM contributed quite a few improvements - especially for Rotational: Clear improvement:
Minor, but unambiguous improvement in my opinion:
There's already a line covering exactly that so it doesn't matter, but to me that indicates it was the intent:
Doesn't matter (covered) - but added for completeness
Could be skipped, but I think it is better:
The rest is more optional Could be discussed:
It matters and it could be that we should add more (other parts have different border colors):
|
…nto the wheel is ends 1mm to the left. Previously it looked as if it was in front of the wheel (when looking at the entire icon - in small scale it doesn't matter). The gradient-part was not changed. Split off from modelica#4711
maltelenz
left a comment
maltelenz
left a comment
There was a problem hiding this comment.
In general, I think this is great!
There are a few cases where I think the old variant was better, and I have commented in those locations/files.
Modelica/Mechanics/MultiBody/Visualizers/Advanced/PipeWithScalarField.mo
Show resolved
Hide resolved
There was a problem hiding this comment.
As for the other machine, I prefer the old (left) more "defined" variant:
If we want to keep the new, the gray rectangle at the top needs to be moved further back in the stack, since it shows through in the current variant on the left edge (if you zoom enough).
There was a problem hiding this comment.
With the two cylinders being aligned and having the same diameter, I like the lack of a black stroke between the colors in the right variant. Perhaps the best would be to combine the cylinder gradients with a common border surrounding both cylinders?
There was a problem hiding this comment.
Will try to add that.
There was a problem hiding this comment.
Have now added it. Please confirm that it looks ok.
There was a problem hiding this comment.
If we want to keep the new, the gray rectangle at the top needs to be moved further back in the stack, since it shows through in the current variant on the left edge (if you zoom enough).
I took care of this, otherwise it looks fine to me.
There was a problem hiding this comment.
I prefer the old (left) one with the clear lines:
This is true for all the differently colored machines, but I will only comment once to keep any discussion organized.
There was a problem hiding this comment.
I tried to see if I could add only the outer line. That gave something like:
annotation (Icon(graphics={
Rectangle(
origin={5.821,15},
fillColor={170,213,255},
fillPattern=FillPattern.HorizontalCylinder,
extent={{-60,-60},{60,60}},
pattern=LinePattern.None),
Rectangle(
origin={5.821,15},
fillColor={128,128,128},
fillPattern=FillPattern.HorizontalCylinder,
extent={{-80,-60},{-60,60}},
pattern=LinePattern.None),
Rectangle(
origin={5.821,15},
extent={{-80,-60},{60,60}}),
Rectangle(
origin={5.821,15},
fillColor={95,95,95},
fillPattern=FillPattern.HorizontalCylinder,
extent={{60,-10},{80,10}},
pattern=LinePattern.None),
Rectangle(
origin={5.821,15},
lineColor={95,95,95},
fillColor={95,95,95},
fillPattern=FillPattern.Solid,
extent={{-60,50},{20,70}}),
Polygon(
origin={5.821,15},
fillPattern=FillPattern.Solid,
points={{-70,-90},{-60,-90},{-30,-20},{20,-20},{50,-90},{60,-90},{60,
-100},{-70,-100},{-70,-90}})}), preferredView="info", Documentation(info="<html>
<p><strong>For a discrimination of various machine models, see <a href=\"modelica://Modelica.Electrical.Machines.UsersGuide.Discrimination\">discrimination</a></strong>.</p>
<p>
Copyright © 1998-2025, Modelica Association and contributors
</p>
<p>This package hosts models for quasi-static transformers.
</p>
<h4>Note</h4>
<p>
Quasi-static DC machines are still operated with DC voltage and current, whereas the quasi-static transformers
are operated with sinusoidal voltages and currents represented by time phasors.
Quasi-static DC machine models therefore are part of the
<a href=\"modelica://Modelica.Electrical.Machines.BasicMachines.QuasiStaticDCMachines\">machines library</a>.
</p>
</html>"));
Giving something like:
I couldn't use github for the diff.
Note I changed the origin of the first one, since it didn't make sense to have different origin for them.
As far as I could see the first Rectangle with moved origin would have extent{...60,61} (meaning that the right-large-cylinder is slightly higher than the left one), but I corrected that to 60,60.
There was a problem hiding this comment.
I applied the same kind of change to the rest of the machines.
Modelica/Mechanics/MultiBody/Visualizers/Advanced/PipeWithScalarField.mo
Outdated
Show resolved
Hide resolved
maltelenz
left a comment
maltelenz
left a comment
There was a problem hiding this comment.
Looks good to me now!
Thanks again @HansOlsson for going through this effort.
And thanks @maltelenz for reviewing and looking at all of them. |
3 participants
Set LinePattern.None for shapes with gradients.
See: modelica/ModelicaSpecification#3789