Hello, there are 2 points where I disagree with you: - breaking the code that works with 2D networks when run on a 3D network is not necessarily a good thing. There is any number of reasonable things to be done with 2D geometry in a 3D network. - controlling return types by parameter is less readable than having dedicated function names. consider reading the following statements in source: pos = node.getPosition(true) vs pos = node.getPosition3D()
Note only is the 'true' surprising (why would a position function have an extra parameter?), it also doesn't reveal what it is doing. By your suggestion, all "clean" scripts would use getPosition(true) from now on. I will stand by the implementation plan 3) from my previous mail of having getPosition() behave as before and getPosition3D() always returning a 3-tuple. regards, Jakob 2016-11-16 11:01 GMT+01:00 sumo.mahei <sumo.ma...@googlemail.com>: > Hi Jacob, > > I was thinking about something like: > > def getPosition('node_id', withZ=False): > if len(self._coord) == 2 and withZ: # fake the non existing > z-coord > return (self._coords[0], self.coord[1], 0) > else: # > return 3d if node is 3d (also for withZ) > return self._coord # return 2d fi > node is 2d and not withZ > > If you want your new code/scripts to be clean, then use withZ=True from > now on. > This always returns x,y,z - save on any net and less code in your script. > > It doesn't break your old code on pure 2d nets, > because your scripts have been eating 2d before and > are feet now again with 2d only since the net is 2d only. > > It will break your old scripts on 3d-networks, > which is __good__ because quite likely you've done > something wrong in the past. > > If that was on purpose, > well then use the 2D-Wrapper with search and replace as you suggested. > > > Am 16.11.2016 um 08:45 schrieb Jakob Erdmann: > > Hello, > > > > 1) the advantage of having 3D and 2D variants of geometry are threefold: > > - it follows the TraCI API which provides 2 distinct methods for > > returning 2D and 3D geometry > > (http://sumo.dlr.de/wiki/TraCI/Vehicle_Value_Retrieval). This is only > > implemented for traci.person.getPosition / getPosition3D in the python > > client at the moment in case you are wondering why you never saw it. > -> I experienced this as a drawback of TraCI, I do more look at SUMO and > netconvert than on TraCI as reference > > - it allows trivial search/replace conversion for existing code > -> agree, see below > > > - it follows the coding style in src/utils/geom with distinct methods > > for handling 2D and 3D geometry > -> not my playground so far :-) > -> we may additionally include wrappers with endings 2D and 3D if that > closes the gap > > > > > 2) the disadvantage of determining function return types at runtime, > > is that it makes code that handles arbitrary networks much harder to > > write (you suddenly need to check types when calling geometry > > functions as your example shows) > -> agree 100% and agian 100% that is why suggested the use withZ=True as > described yesterday and again above for arbitrary net-scripting > b.t.w. checking always the dim is truly very annoying and that is the > reason why I did write some sumolibExtensions some time ago. > > > 3) you raise an important point regarding backwards compatibility. > > I'm not sure how many people use tuple unpacking in their code but I > > agree that it's bad to break working code. > -> ??? you can never tell :-) I know you did use unpacking in your > scripts :-) > > > An alternative would be to have the old client methods continue to > > return 2D geometry and only add a new method with the 3D suffix > > (exactly as in traci.person but somewhat different than in > > src/utils/geom). > > > -> this might let your (future) users to stumble across the missing > z-coord where they expect them. They might think this is a bug unless > they dive into the docs > to find out. Not the nicest coding experience. > > > best regards, > > Jakob > > > > > > So which way to go? > I suggest: > > * return geometry tuples according to the largest dim of the net > (either always 2d on a pure 2d net or always 3d on a net with at > least one node's z!=0) > -> requires a tiny post process after loading the xml data. > > * geometry return can switch on the z-layer optionally for 2d nets for > ease of coding > > * have getSomething2D() and getSomething3D() - Wrappers for all geometry > returning functions, > brute forcing 2d and 3d tuples even if information is lost. > > Would that be fine for you? > > > Best Marek Heinrich > simoserv GmbH > > > > > > > > > > > > > 2016-11-15 19:11 GMT+01:00 sumo.mahei <sumo.ma...@googlemail.com > > <mailto:sumo.ma...@googlemail.com>>: > > > > Dear Jacob, > > > > maybe it's worth putting the discussion about Ticket 2072 > > from private mailing to the sumo-devel list. > > > > We are currently facing three alternatives for > > introducing 3d (x,y,z)-coordinates to the sumolib: > > > > Alternative 0 - sumolib resemble the sumo.net.xml/netconvert > behaviour > > ============================================================ > ========== > > > > netconvert has the ability to eat z-coordinates during the > > netbuilding process for z=0. > > > > This is how SUMO/netconvert is, even though I find this annoying > > whenever > > I come along this (often enough). > > Here is an example, which is also part of yesterdays > > sumolib3d-unittest: > > > > The node-file includes 3D-coordinates: > > > > <node id="first" x="100.0" y="0.0" z="0.0" /> > > <node id="second" x="200.0" y="0.0" z="10.0" /> > > > > Once netconvert has finished it's work, the sumo.net.xml contains > > for instance the following items: > > > > <junction id="first" type="priority" x="100.00" y="0.00" > > incLanes="the_other_way_0" intLanes=":first_0_0" shape="100.00,-0.05 > > 100.00,-2.05 100.00,2.05 100.00,0.05"> > > > > <junction id="second" type="priority" x="200.00" y="0.00" z="10.00" > > incLanes="this_way_0" intLanes=":second_0_0" > > shape="200.00,0.05,10.00 200.0\ > > 0,2.05,10.00 200.00,-2.05,10.00 200.00,-0.05,10.00"> > > > > <lane id="this_way_0" index="0" speed="13.89" length="100.50" > > width="2.00" shape="100.00,-1.05 200.00,-1.05,10.00"/> > > > > As you can see here, the 'first' point is pure 3d with z=0 and > > results in a pure 2d 'first' junction. > > > > No so with 'second' as z is different from zero. > > > > The lane 'this_way_0' is 2.5d since it starts its shape with a > 2-tuple > > and continues in full 3d. > > > > I can fully understand that this helpful for backward compatibility , > > but it also introduces extra work, e.g. when processing > > sumo.net.xml files > > in third application. > > > > Fixing this issue in netconvert is not in my scope and probably > > also not > > in the scope of the ticket :-) and maybe even a different > discussion. > > > > My conclusion about alternative 0: > > I believe it's normal to expect the sumolib to behave like > > sumo/netconvert > > itself, even though I don't like this behaviour. > > > > > > Alternative 1 (dlr-team): > > ========================= > > > > This is how I do understand your amendment to Ticket 2072: > > > > any geometry functions should always return 3-tuple, regardless of > the > > dimensions of the net - also for 2d-nets. > > > > Pro: This is great for all nets that contain 3d-Nodes. > > Pro: This makes all users and developers of new script aware > > that they might consider 3d-Networks as well > > > > Con: This will break all existing scripts, once the user do > > update to a new version of SUMO, since up to now > > never 3-tuple were returned even on 3d-networks. > > This will result in thousands of: > > ValueError: too many values to unpack > > > > This will also break all strips running on 2d-networks. > > Even though it's perfectly fine to return 2-tuples on a > > 2d-Networks (Node only having x,y coords only). > > > > My personal feeling is that most Networks are in 2d. > > > > My conclusion about alternative 1: > > This is a lot of avoidable trouble and extra work for your > > users and coders, so please don't stick to it. > > > > Alternative 2 (mixture): > > ========================= > > > > Why not loading all nodes, and keep a record on the net-object > > about its dimension. > > > > As long as there are no 3d-nodes read by startElement() > > the net remains as 2d net. > > -> net has either only 2d nodes or all 3d-nodes are in xy-pane. > > > > Once the first node with three coords is read, the net property > > 'dimension' is changed from 2 to 3. > > > > After finishing reading all nodes from xml, the > > nodes will be processed and missing z-coords will be filled with > > z=0. > > > > If the user accesses now geometry values she/he will be returned > > either 2- or 3-tuple, depending on the net he is loading. > > > > Rather then having a 2D-Wrapper (see below) i would suggest > > a option 'with_z=True' returning optional the missing z=0 coord on > > a 2d net. > > > > btw. in python3.5 this recipe would work: > > x,y,z = net.getCorrds() if net.getDim() == 3 else *net.getCoords(), 0 > > > > > > A word about backward compatibility with 2D wrapper > > ================================================ > > > > I don't see the point in having a extra function getShape2D() > > or getPositon2d() and so on. > > > > As already in the ticket's amendment described, it's as easy > > for a sumolib user to adapt the code > > from: > > x,y = getSomething() > > > > to: > > x,y,z = getSomething() > > > > it is more straight forward - even if z as > > variable is never used again > > > > I would argument that adding a wrapper whatEverFunction2D() > > to the whatEverFunction() might be of little effort, but still > > in my opinion it will introduce dead code - But this is at your > > choice. > > > > > > Geometries in TraCI > > =================== > > B.t.w. Traci is more strict, it returns 2d coords even for 3d-nets. > > > > > > So which way should we travel? > > > > Best Marek Heinrich > > simoserv GmbH > > > > > > ------------------------------------------------------------ > ------------------ > > _______________________________________________ > > sumo-devel mailing list > > sumo-devel@lists.sourceforge.net > > <mailto:sumo-devel@lists.sourceforge.net> > > https://lists.sourceforge.net/lists/listinfo/sumo-devel > > <https://lists.sourceforge.net/lists/listinfo/sumo-devel> > > > > > > > ------------------------------------------------------------ > ------------------ > _______________________________________________ > sumo-devel mailing list > sumo-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/sumo-devel > ------------------------------------------------------------------------------ _______________________________________________ sumo-devel mailing list sumo-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/sumo-devel