Unify location of connectors in Electrical libraries

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.

https://doc.modelica.org

BSD 3-Clause "New" or "Revised" License

479 stars 169 forks source link

Unify location of connectors in Electrical libraries #2215

Closed christiankral closed 7 years ago

christiankral commented 7 years ago

The electrical libraries use sometimes connector locations which are not in line with the general look and feel experience of other packages of the MSL

Electrical connectors are slightly displaced from the icon boundary (e.g. Modelica.Electrical.Analog.Basic.M_Transformer)
Electrical connectors are not on a strict grid of 10% grid (with respect to 100% icon width and height) which causes the connections from these components to show "unmotivated" edges (e.g. models based on Modelica.Electrical.Analog.Interfaces.TwoPort)
Real input connectors are not placed at the icon limit (e.g. Modelica.Electrical.Analog.Basic.VariableResistor); a reference implementation is Modelica.Blocks.Interfaces.SISO
Boolean inputs are placed on different horizontal and vertical locations for no logical reason (e.g. Modelica.Electrical.Analog.Ideal.IdealCommutingSwitch, Modelica.Electrical.Analog.Ideal.IdealThyristor)

I propose to unify then location of connectors to give the icons a more professional look. This issue is somehow also related to #2031 and #2210, as the change of connector locations could also enable a unification of the inductor icons. I will create a new branch to provide a pull request.

christiankral commented 7 years ago

The %name is also slightly displaced from on model to another.

christiankral commented 7 years ago

Some components have a connector size slightly deviating from the standard size; this will be corrected, too.

dietmarw commented 7 years ago

@christiankral some comments:

Yes it would be good to unify all this but according to which guidelines? You should provide proper guidelines at the same time that also other then can follow. Otherwise we get new models by other people that use their personal taste that might (and probably will) deviate from what you consider best.
1. Rather then creating branches here at the upstream repository it would be better (and is the normal way) that you create a branch for the pull-request in your fork. That way the main repository does not get "cluttered" with lots of branches where people try out things to work in etc.

christiankral commented 7 years ago

Created branch https://github.com/modelica/Modelica/tree/christiankral_Connectors%232215 where I tried to unify

name and parameter locations and sizes (as far as possible)
icon and parameter colors
locations of connectors and their sizes
inductor and capacitor shapes (Analog, Machines, MultiPhase, QuasiStationary.SinglePhase, QuasiStationary.MultiPhase) -- related with #2210 and #2031
shapes of transistors, diodes, operational amplifiers, switches

In general the icon size was rather extended to the full available space such that the icons and their texts improve readability.

I will double check my proposal before providing a pull request, but you may already have look to check whether you like these unifications: @kristinmajetta @AHaumer @dietmarw @beutlich

beutlich commented 7 years ago

The icons of Modelica.Electrical.MultiPhase.Ideal.IdealDiode, Modelica.Electrical.MultiPhase.Ideal.IdealThyristor and Modelica.Electrical.MultiPhase.Ideal.IdealGTOThyristor have a black diode though I recently changed Modelica.Electrical.Spice3.Semiconductors.D_DIODE from black to blue. Which one is to prefer?
Should the icons of Modelica.Electrical.QuasiStationary.SinglePhase.Sources.* also be light blue instead of black?
Should the icons of Modelica.Electrical.QuasiStationary.MultiPhase.Sources.* also be light blue instead of black?

christiankral commented 7 years ago

I changed the of color from black to blue of the Analog library in https://github.com/modelica/Modelica/tree/christiankral_Connectors%232215

I also unified the colors of QuasiStationary components...

I am also planning to propose guidelines for icons, as Dietmar suggested. Work in progress...

christiankral commented 7 years ago

The unification of icon connectors also includes the unification of the entire icon design. So I added "Icon Design" guidelines in Modelica.UsersGuide.Conventions.Icons. I tried to make the guidelines simple and I did not want to entirely change the existing icon design. The proposed guidelines are a compromise but overall straighten the look and feel experience.

In addition to the "Icon Design" guidelines I provided re-designed icons of these libraries in https://github.com/modelica/Modelica/tree/christiankral_Connectors%232215:

Modelica.Electrical.Analog
Modelica.Electrical.MultiPhase
Modelica.Electrical.QuasiStationary.SinglePhase
Modelica.Electrical.QuasiStationary.MultiPhase

christiankral commented 7 years ago

I am done with my proposal for now. So I am mainly asking the library officers @kristinmajetta @AHaumer and others to have a look and initiate discussion. Please provide you feedback.

beutlich commented 7 years ago

There is a typo in the User's Guide: siginifcant -> significant

christiankral commented 7 years ago

Fixed typo in a09ef10

dietmarw commented 7 years ago

One thing that always puzzles me how to get right is the placement and size of the text. It would be nice to have some proper placement and size, preferable with coordinates that are symmetrically and perhaps starting and ending on a multiple of 10 of the grid. Would be nice if this can be taking into consideration.

beutlich commented 7 years ago

I'd propose an extent height of 40 and black color for the text. Currently it is different in every library.

beutlich commented 7 years ago

@christiankral Is #2117 also addressed by your proposed changes?

christiankral commented 7 years ago

Yes, the multi phase power sensor is included in my proposal.

christiankral commented 7 years ago

@dietmarw I included guidelines for the icon design which includes, text size, text location etc. in Modelica.UsersGuide.Conventions.Icons. Do you think this is sufficient?

dietmarw commented 7 years ago

@christiankral Perhaps change the grid resolution to 10 for the screen shots so that the names don't look floating (even though they are on the 10 res grid positioned).

dietmarw commented 7 years ago

Also, maybe centre align the figures and the captions ?

christiankral commented 7 years ago

@dietmarw

I kept the grid resolution of 20 (which is a usual setting) and added dimensions in 68f9ec6, instead
I am fine with the left aligned figures as we do not have centered figures in the guidelines Modelica.UsersGuide.Conventions.Documentation.Format.Figures

dietmarw commented 7 years ago

beutlich commented 7 years ago

Just another typo: Comonent -> Component

christiankral commented 7 years ago

Fixed typo in 65488df.

beutlich commented 7 years ago

There already is some information on icons in Modelica.UsersGuide.Conventions.ModelicaCode.Format. I guess it needs to be merged properly.
Should Modelica.UsersGuide.Conventions.Icons be moved to Modelica.UsersGuide.Conventions.ModelicaCode.Icons?
Inheritance from Modelica.Icons.Information is missing.

beutlich commented 7 years ago

Those very long (default) names for components do not look that nice and also different between tools, see e.g., Modelica.Electrical.Analog.Ideal.ControlledIdealIntermediateSwitch.

simx dymola mworks sysplorer

I see two solutions

Provide a shorter default component name, as e.g., already done for many models in Modelica.Electrical.Machines.
Increase extent width of text field

christiankral commented 7 years ago

I think the logical location of the "Icon design" shall be Modelica.UsersGuide.Conventions.Icons, not Modelica.UsersGuide.Conventions.ModelicaCode.Icons, as the icon design is more general than "coding"; therefore I removed the obsolete comment on icons in 5940acb344783cb4ed0171f5b83e237cacb7b200
I added extends from Modelica.Icons.Information in 398e018

christiankral commented 7 years ago

Default component names: I am very much in favor of applying a default component name such as "switch" as we have different icons to distinguish the classes. I think that extending the text field does not really improve readability, as there is always one name too long. An additional drawback of extended text fields is, that the overall icon size gets bigger, which is rather a drawback considering "limited" drawing space.

beutlich commented 7 years ago

There is a odd sentence in Modelica.UsersGuide.Conventions.ModelicaCode.

Additionally some format UsersGuide are stated.

beutlich commented 7 years ago

In Modelica.UsersGuide.Conventions.ModelicaCode.Naming, sixth bullet there could be a link from "icon layer" to Modelica.UsersGuide.Conventions.Icons.

beutlich commented 7 years ago

Looks good to me. Thank you for your contribution!

beutlich commented 7 years ago

@christiankral Ready to merge, beside the shorter default component name?

christiankral commented 7 years ago

@beutlich Yes its OK from my side.

christiankral commented 7 years ago

I will apply the default component names (also to magnetic components) at a later time.

christiankral commented 7 years ago

@beutlich Could you do the merge, please? Thanks in advance.

christiankral commented 7 years ago

The proposed suggestions specifically also matter to @kristinmajetta

@kristinmajetta: Please check whether you agree to this proposal or not, as many icon changes in the Analog package have been implemented.

beutlich commented 7 years ago

I squashed your commits to master, deleted the dev branch and also informed @kristinmajetta again about the progress.