webots
webots copied to clipboard
cyberbotics
New Python API
Fixes #2385.
This PR aims at implementing a new python controller API for Webots replacing the previous one with the following advantages:
- Don't depend on swig to generate the python library.
- Add python type annotations.
- More "pythonic" style: use of python getters and setters.
- Use Python constructors to instantiate devices instead of the
Robot.get_device()method. - Rely on libController instead of libCppController: simpler to maintain (especially data type conversions), less dependencies (like libstdc++) and more lightweight (no need to load libCppController, no overhead in calling C functions through C++ API).
- Use of doc-string.
- Various improvements of the API (including the supervisor API for managing nodes and fields).
- We could get rid of the various lib/python37, lib/python38, lib/python39 folders and keep only one lib/python folder.
- And probably more...
Edit: For anyone arriving here late who doesn't want to scroll down through what has now become a great wall of text, here is a link to my current draft of this New Python API. It's still fairly rough, but is quite usable already, and will keep improving. Bug reports and suggestions welcome!
https://github.com/Justin-Fisher/new_python_api_for_webots
Continuing discussion from #2358.
The proposed export mechanism to make constants available in .dll's looks like it would be useful, and this would provide a nice means for other languages' API's to always get up-to-date values for the constants.
I wasn't sure about the names you export them as though. In the C documentation, these are given names beginning upper-case WB_ so I think that would lead someone who uses a .dll version of the C API to expect that these constants would have names beginning 'WB_', whereas you seem to export them with names that instead start lower-case 'wb_'.
Is there a unified list of all Webots constants somewhere (or a list of all the ones that we'd want to export for use in other languages' API's anyway)? This could be a useful checklist if you're looking to add similar exports to the remaining constants. If not, I have available a list of how all the constants appear in the swig/C++ version, and I have an algorithm that does pretty well at translating those names into the C naming scheme which matches the documented names for the dozen or so that I manually checked, but if any constants have idiosyncratic names this algorithm might not handle them correctly, and without a list to check against I don't have a good way to be sure, short of trying to go look them all up in a bunch of different places, which I'm not eager to do. Even if my list isn't perfect, it might still be a useful checklist.
I wasn't sure about the names you export them as though. In the C documentation, these are given names beginning upper-case WB_ so I think that would lead someone who uses a .dll version of the C API to expect that these constants would have names beginning 'WB_', whereas you seem to export them with names that instead start lower-case 'wb_'.
I believe both versions are needed. the wb_ version is a symbol (similar to API function which starts with wb_) whereas the WB_ version is an enum (similar to a define) defined in the header file. The enum version is still needed because it is more convenient to use in C/C++. For example it is not possible to perform:
switch(node_type) {
case wb_NODE_CONE:
...
Because wb_NODE_CONE is unknown at compile time, whereas it is possible with WB_NODE_CODE as an enum (or define).
Also, is the roundabout import into Python using ROTATIONAL = ctypes.c_int.in_dll(_wb, 'wb_ROTATIONAL').value really necessary? I was under the (perhaps mistaken) impression that for vanilla types like ints, ROTATIONAL = _wb.wb_ROTATIONAL would work just fine.
Incidentally, I've set my version up to have a small handler handle API calls so (at least for now) I could use the swig/C++ names for API methods and have the handler automatically translate them to the corresponding C-name (and cache the results so there's no notable performance hit). This approach would support the fairly attractive notation of using _wb.ROTATIONAL as being shorthand for what I repetitively wrote as _wb.wb_ROTATIONAL above, or you wrote as ctypes.c_int.in_dll(_wb, 'wb_ROTATIONAL').value.
The list of constants is computed by the matlab script and they will appear as files in the matlab folder with uppercase filenames (not on github, but in your local installation).
I believe both versions are needed. the
wb_version is a symbol (similar to API function which starts withwb_) whereas theWB_version is an enum (similar to a define) defined in the header file.
If one version will be used only under the hood in the C source code (will it?), and the other version will be exported for other people to use, then I would suggest making the version that gets exported have the name that matches the documentation. E.g., call the enumerated local version ROTATIONAL, and call the exported version WB_ROTATIONAL. Or you could change the documentation to note that these will have wb_ names rather than WB_ ones.
Also, is the roundabout import into Python using
ROTATIONAL = ctypes.c_int.in_dll(_wb, 'wb_ROTATIONAL').valuereally necessary? I was under the (perhaps mistaken) impression that for vanilla types like ints,ROTATIONAL = _wb.wb_ROTATIONALwould work just fine.
I just test it but unfortunately, it doesn't work... We have to rely on the more complex version.
If one version will be used only under the hood in the C source code (will it?), and the other version will be exported for other people to use, then I would suggest making the version that gets exported have the name that matches the documentation. E.g., call the enumerated local version
ROTATIONAL, and call the exported versionWB_ROTATIONAL. Or you could change the documentation to note that these will havewb_names rather thanWB_ones.
The wb_ names should never be visible to the users, as users will use the object constant without any prefix, e.g., Motor.LINEAR, not wb_LINEAR. So, the wb_ version doesn't need to be documented in the API. It will be used only in the python module (and possibly in other language interfaces in the future).
The more explicit type conversion like that we have to do, the more annoying it will be to write the Python controller manually, though I can probably jerry-rig a system that hides these extra steps under a convenient notation.
For some reason, the _controller.pyd version of a dll doesn't have these sorts of annoying type-casting issues. E.g., controller.py's analog to your line is just ROTATIONAL=_controller.Motor_ROTATIONAL. I don't really understand all the ways that '.pyd' differs from .dll. Would there be a prospect for making a '.pyd' version of the C-API to import into our new python controller? Might end up being more convenient if it lets us avoid manually recasting a lot of types. (Or it might be more trouble than it's worth. dunno...)
Edit: Reading up a bit more, it looks like .pyd is just an alternate extension for a .dll that defines some stuff to make it easier to import into python, presumably including some cues that indicate the types of constants. Swig probably automatically added those pieces, but as we move away from swig, we probably wouldn't want to worry about adding them to a .dll. So we'll probably be stuck with a fair amount of explicit type-setting, including for constants.
The more explicit type conversion like that we have to do, the more annoying it will be to write the Python controller manually, though I can probably jerry-rig a system that hides these extra steps under a convenient notation.
This will be annoying only for us, developing the webots python module, not for users, but if you can jerry-rig something to make this smoother, that would be nice.
For some reason, the
_controller.pydversion of a dll doesn't have these sorts of annoying type-casting issues. E.g., controller.py's analog to your line is justROTATIONAL=_controller.Motor_ROTATIONAL. I don't really understand all the ways that '.pyd' differs from.dll. Would there be a prospect for making a '.pyd' version of the C-API to import into our new python controller? Might end up being more convenient if it lets us avoid manually recasting a lot of types. (Or it might be more trouble than it's worth. dunno...)
Yes, but generating a .pyd file is more complicated and platform dependent, probably also python-version dependent. This would kill advantage 8 listed in the first comment and would yield to a more complex build process. I would be happy to avoid that.
I just added a helper function to make it easier for us to retrieve constants.
So I've extracted all the constants from the Matlab controller, and I've gone through all the constants that were used in the swig-produced controller.py, and worked out systematic ways to translate their names to the corresponding Matlab names which themselves claim to have been straight from the documented names. Unfortunately, I did run into the following exceptions (note, for simplicity, I'm leaving out the WB_ prefix throughout):
| swig constant | expected translation |
|---|---|
| 'Node_ALTIMETER' | 'NODE_ALTIMETER' |
| 'Field_MF_ROTATION' | 'MF_ROTATION' |
| 'Supervisor_MOVIE_READY' | 'SUPERVISOR_MOVIE_READY' |
| 'Supervisor_MOVIE_ENCODING_ERROR' | 'SUPERVISOR_MOVIE_ENCODING_ERROR' |
| 'Supervisor_MOVIE_SAVING' | 'SUPERVISOR_MOVIE_SAVING' |
| 'Supervisor_MOVIE_WRITE_ERROR' | 'SUPERVISOR_MOVIE_WRITE_ERROR' |
| 'Supervisor_MOVIE_RECORDING' | 'SUPERVISOR_MOVIE_RECORDING' |
| 'Supervisor_MOVIE_SIMULATION_ERROR' | 'SUPERVISOR_MOVIE_SIMULATION_ERROR' |
I think the first two are probably errors of omission in the Matlab controller, since it doesn't seem to define anything like these, so perhaps these should be added there? I think the remaining ones have to do with the C++/swig implementation of supervisor movie control, which would probably have to be re-implemented in a manually written Python controller anyway? Or maybe these are further errors of omission in the Matlab controller?
So, anyway, I've now caught us up to the Matlab controller regarding constant-importation. Whenever constants get changed, the Matlab controller would have to be manually updated. If we're happy enough having the new python controller be like that too (at least for now), then I've got a copy of the matlab constants ready for us to use, and I can automatically-rewrite the swig controller.py to use these. (Of course, it would be nice eventually to have both Matlab and Python import current values for these constants, so that they wouldn't require manual updating whenever constants change.)
Another quality-assurance check I should probably do, since it'd be pretty easy, is checking whether MATLAB and swig/C++ actually do assign the same values to all the constants!
I believe the problems come from the scoping of these constants. In C and Matlab, no scoping is possible hence all the constants are defined at a global scope. In OO languages, such as C++, Java and Python, the constants are scope with objects, e.g., Supervisor.MOVIE_READY or Field.MF_ROTATION which looks better. As a consequence there is no one-to-one match between the C/Matlab implementation and the C++/Java/Python implementations.
However, there is not a huge number of constants and I believe that a minimum of manual maintenance for them is acceptable, as it would be quite difficult to automate this fully.
Yes, much of the automated name-translation system I came up with was translating between the Matlab/C naming system that keeps everything at global scope, and the swig naming system which also transfers the constants from C++ to Python using constants with global scope, but uses different naming conventions to reflect which local scope each constant will live at in the C++/Python API's.
The exceptions I listed above aren't just further examples of this. The Matlab API doesn't define anything at all for these constants, not just some different way of naming them. The only Matlab constants that don't have analogs among the swig constants are WB_STDERR and WB_STDOUT.
It looks to me like the Matlab API doesn't offer any way of telling whether a node is an Altimeter by comparing it to a constant like WB_NODE_ALTIMETER. As far as I can tell this is just an oversight in the Matlab controller. This should probably ring alarm bells, since the other node types are all created from an enumerated list, so if that list is missing Altimeter, it may be that everything after that in the enumeration is off by one!!! (The docs do say that this should be an enumerated list including Altimeter, so my bet is that the Matlab constants for almost all of the node types -- everything after altimeter on the alphabetized list -- are just wrong!)
Similarly, the Matlab API doesn't define WB_MF_ROTATION even though it does define the analogous WB_SF_ROTATION. I'm not sure how many MF rotation fields there actually are (how often do you want an arbitrarily long list of rotations?), so this may not be all that big of an oversight, but it seems like it probably still is an oversight. Fortunately these ones are assigned values manually, rather than being enumerated, so this likely won't have thrown off the numbering for anything else.
The various MOVIE constants in the swig API look to me like they're probably a remnant from some deprecated way of checking movie statuses, redundant with and superceded by the currently documented movie functions. Since the Python and C++ methods that use these constants (Supervisor.getMovieStatus and its synonym Supervisor.MovieGetStatus) are no longer documented, I think it'd probably be fine for us to abandon them and the associated constants.
Ok, I have compared the constant values produced by the swig/C++ controller to the corresponding constant values produced by the Matlab controller.
The good news is that most of these match up perfectly, which confirms that my name-translation system is working well, and that the list of constants I've produced is mostly right.
The bad news is that most of the node-type constants are one smaller in the Matlab version than they are in the swig/C++ version of Webots, which, as I speculated above, probably stems from the fact that Matlab left WB_NODE_ALTIMETER out of its enumeration. So In this case, I suspect that the Swig/C++ constants are correct, and the Matlab Controller should be amended to include WB_NODE_ALTIMETER.
Edit: I submitted a PR #3808 to fix the constants in the Matlab generator.
Edit: I've now added to this branch a simple module WB_CONSTANTS.py that defines all the constants to their currently-correct values using their documented C names, except with a leading underscore (e.g. WB_NODE_GPS became _WB_NODE_GPS).
I've now added a line from WB_CONSTANTS import * in the skeleton version of webots.py. This allows this skeleton to refer to any constant whose documented name is WB_NAME as _WB_NAME.
(I had originally committed a version that would have had us refer to the C constants as _WB.NAME but ended up deciding that there was no real advantage of it over the current version, and this version will be a teensy bit more performant. That means we no longer need it to have the opaque name _WB.py, so I changed it to the more easily understandable WB_CONSTANTS.py)
Would it make sense for me also to upload the script that I used to generate this (based off the Matlab script, but with its errors fixed)? If a new device is inserted into the long enumeration of device types, it'd probably be slightly easier to modify this script than it would be to manually renumber everything. But, on the other hand, manually editing this takes just a text editor, whereas changing and running the script requires a bit of python knowledge, and if Person A makes some manual changes to WB_CONSTANTS, and then Person B reruns the script, Person A's manual changes would be lost. (Eventually, we might hope this will be all made moot, if the C .dll can be made to export uptodate constant values.)
Traditionally Webots' Python API has constructed devices via a generic Robot.getDevice(name) or specific versions like Robot.getMotor(name). The most "pythonic" way of creating a new Device/Motor object would probably be with Device(name) or Motor(name) and it looks like you're supporting this more pythonic usage in the skeleton controller. I'm happy supporting this more pythonic usage, with the slight reservation that it requires importing Device and/or specific device types like Motor, not just Robot which is the only thing that many existing controllers import.
For reasons of backwards compatibility, I think we should also continue to provide at least Robot.getDevice(name) and perhaps also undeprecate Robot.getMotor(name) and all its siblings, since these will make type hinting and device-specific documentation strings a bit more convenient. These will probably end up just being very-longhanded references to Device or its various subclasses like Motor.
I'm also tempted to provide Robot.Motor as a slightly less longhand way of referring to Motor (and similarly for other device types). I think many people begin their controllers just with from controller import Robot and it would be nice to have that provide a way of referring to device types without needing to explicitly import them.
On this proposal, the recommended documented usage would be the following, since it is the shortest option that provides an implicit type hint without needing any extra imports beyond Robot:
m1 = Robot.Motor('motor1')
However, all of the following would be fully equivalent to the above (though the various Motor versions would provide more specific docstrings than the Device versions would):
`m1 = Robot.getDevice('motor1') # type: Motor #backwards compatible, but requires Motor import for explicit type hint
`m1 = Robot.getDevice('motor1') # type: Robot.Motor # explicit type hint doesn't need extra import
`m1 = Robot.getMotor('motor1') # currently "deprecated" but supported, will get its type hint implicitly
`m1 = Robot.Device('motor1') # type: Motor #explicit type hint requires importing Motor
`m1 = Robot.Device('motor1') # type: Robot.Motor #explicit type hint doesn't need an extra import
`m1 = Robot.Motor('motor1) # arguably the best option: shortest implicit type hint with no extra import
`m1 = Device(`motor1`) #type: Motor # requires Device import, and explicit type hint also requires Motor import
`m1 = Motor('motor1`) #shortest possible implicit type hint, but requires Motor import
I didn't bother listing them, but if you name your Robot robot, then you would also get the same results replacing Robot with robot in each of the above.
Under the hood, this would be implemented by making Robot.getDevice and Robot.Device both refer directly to Device, making robot.getMotor and robot.Motor refer directly to Motor (which is a subclass of Device so inherits Device.__init__), and so on. All of the work of setting up the new device will then be done by Device.__init__, with the only real contributions of the other aliases being, (a) that they enable implicit type hinting of specific device types, (b) they will provide documentation strings about the specific device types, and (c) the Robot.foo versions don't require any additional imports beyond just Robot.
So, that's how I'm tentatively planning to proceed, unless I hear objections?
Regarding constants, I don't see the point of having this WB_CONSTANTS.py file where we need to manually (or via a python script) maintain values that are redundant with the ones hard-coded in the libController (library or header version). The mechanism I developed (e.g., Motor.ROTATIONAL) is compatible with the current version and avoids this redundancy of hard-coding the same values at several places. Isn't it satisfactory?
So, that's how I'm tentatively planning to proceed, unless I hear objections?
About explicit imports, can't we consider that:
from webots import *
should be used by those who don't want to bother about importing each device class explicitly.
However, I am pretty sure that some developers will prefer the form:
from webots import Robot, DistanceSensor, Motor
because they have full control on what gets imported and how this will affect their global namespace.
About backwards compatibility, we are going to break it anyhow, so shouldn't bother about that.
I don't like too much the idea of having many different ways to get a device. That means we should document, maintain, test and support them all, which looks overkill to me.
So, I believe we should identify what is the best way to getting a device and implement only this one.
Currently my favorites are:
m1 = Motor('motor1') # shortest possible implicit type hint, but requires explicit or implicit Motor import
m1 = Robot.Motor('motor1) # shortest implicit type hint with no extra import
If from webots import * has no major drawback over from webots import Robot, I would tend to prefer the first one.
Maybe I'm mistaken, but my understanding was that someone would need to go through a bunch of different C files to make them export all the relevant constants, I wasn't sure we had anybody who wanted to do that, so I had thought that assigning all the constants correct values myself would work as a stopgap until someone got around to making the C constants export themselves. Generating this was super-easy as a byproduct of work that I was doing anyway to confirm that I had the correct translations from the swig way of referring to constants to the C way. Having them generated this way seemed like a good enough solution for now, equally good to the solution that the Matlab controller has been using.
Once we do have a fully working system that exports all the constants from C, then yes I agree that the Matlab and Python API's should both use that rather than trying to reconstruct the constant values like it and I did. But I didn't feel confident that anyone was going to produce that any time soon, since it's probably a relatively low priority to do a bunch of work now to avoid doing a little work with each subsequent Webots update. So I was instead giving us a solution that'll work well enough for now so that we can focus on the other challenges in manually writing a Python controller. But if you or someone else can get all the C constants to export themselves, it should be easy to switch.
There are not too many constants to export from C and I am happy to do this, so you can consider this will be done early next week.
If
from webots import *has no major drawback overfrom webots import Robot, I would tend to prefer the first one.
from ... import * is generally discouraged since it introduces the potential for lots of namespace collisions, including unforeseeable ones that might arise at some point down the road if a future version of Webots introduces a new entity with the same name as some entity that someone had used in their controller, which at the time didn't cause a collision.
For that reason, it's generally preferable to either use import controller and refer to everything as controller.foo or to use from controller import ... and list just one or a few essential things to import, like Robot. It would be a real pain to keep adding more imports to that list every time you add a new device-type to your robot. So better to keep it as it has been, so importing Robot (or Supervisor) gives you everything you really need for 90%+ of what people are gonna do, so they don't have to keep adding to their list of imports, and also don't need to worry about any present or future namespace collisions.
About backwards compatibility, we are going to break it anyhow, so shouldn't bother about that.
Are you sure about that? I had been approaching this with backwards compatibility in mind as a hard constraint, with the thought that, at worst, I might put deprecation notices on various things that we'd like to phase out. I had thought that I'd be able to do pretty much everything I wanted to do within that hard constraint. What sort of breaks in backwards compatibility do you think we need?
Anyway, *if* we were to view backwards compatibility as negotiable, but still want to stay somewhere in the ballpark of the old approach to help ease whatever switchover is needed, then I think I would recommend continuing to make it possible to do virtually everything with just from controller import Robot (or Supervisor) to save users the hassle of getting all the import lines right, and to avoid potential namespace collisions. I would stick with having a Device superclass with various specific subclasses like Motor. I would make their __init__ methods work so that, Motor(name) would be a fine pythonic way of creating a Motor object with the given name. But since I'd want people to be able to access all this via a single Robot import, I would define Robot.Motor to be an alias for Motor, and I would make the documentation recommend using m1 = Robot.Motor("motor1") as the single suggested way of getting a Motor device, though at some point I would likely also note that Robot.Motor returns the Motor device subclass.
If we were to not aim for backwards compatibility, we wouldn't necessarily need a generic Device constructor (like Robot.getDevice) but there are some occasions where this could be useful, e.g. if you're writing code to flexibly handle robots with different devices, or if you're not using an IDE that does type-hinting, and find it easier to just use a single function for all devices. We'll also need to write most of this anyway if we're going to continue supporting dynamically getting devices by index, which it seems like we probably should. For these reasons, along with backwards compatibility, I'd still be inclined to make a generic Device constructor anyway, and a good Pythonic name for it would be Device(name_or_index), again with the alias Robot.Device(name_or_index) to enable use of this just by importing Robot. I probably wouldn't make a point of mentioning this in the documentation for each particular device, and instead would probably just note it in the generic Device docs python section and where wb_robot_get_device_by_index is documented. (An alternative quite pythonic option for getting devices by index would be robot.Device[index], which I could easily set it up to support.)
The only difference between this "setting aside backwards compatibility" ideal design and the design I proposed above is that the design I proposed above also gave us full backwards compatibility by adding a few lines to the Robot class definition to set getDevice = getDeviceByIndex = Device and getMotor = Motor (and similarly for the other device subclasses). This seems like a very small price to pay to achieve backwards compatibility. I would recommend *not* documenting Robot.getDevice, Robot.getMotor and the like, if you'd like to phase them out, or documenting them as being still usable but deprecated. We could also make their usage generate deprecation warnings if you'd like to clear the path towards phasing them out entirely. (getMotor and the other specific ones already do generate deprecation warnings, so you might feel comfortable phasing them out sooner than you would the not-yet-deprecated getDevice, even though I'd actually more recommend using getMotor than I would getDevice, since it'll play nicer with linters.)
There are not too many constants to export from C and I am happy to do this, so you can consider this will be done early next week.
Cool. Whenever you have it working, I'll rewrite our Python code to import your exported constants in the relevant places. I have scripts set up to introspect and manipulate the code (including a version of controller.py that I'm half-done converting to the C-API), so making scripted changes like this is pretty easy.
I understand the Matlab generating script well enough that I could alter it too, though for that it would help if you could also export a full list of constants (since Matlab basically just imports all of them, keeping their names intact)?
I don't yet have a sense of the relationship between the C++/Java/ROS controllers and these constants. I suppose it might be a good idea for someone to alter them to do such imports too, if they don't already.
Incidentally, another approach that you might consider would be setting up a script that would scrape all the constant definitions out of the relevant C-files. This might be less work than manually changing it to export them, and would have the added benefit of generating a list of all the constants, which would be helpful for thin wrappers like the Matlab API. Such a script could output the constants as a simple file like the WB_CONSTANTS one I made, and/or it could generate C code to export them in the .dll.
controller.py redundantly defined numerous functions both as static methods of device subclasses (e.g., Speaker.playSound) and as stand-alone functions (e.g. Speaker_playSound). These amount to two different names for the same thing, the former of which is documented, whereas the latter of which isn't. The latter seems likely to just be an auto-generated swig artifact that probably never got used by humans much, if ever. I guess if you're less concerned about backwards compatibility than I am, you won't mind if I abandon the latter (stand-alone) versions of these?
controller.py defined methods Robot.getData and Robot.setData that aren't documented and don't seem to have associated C-API functions (at least not ones that are documented). I'm not sure if these are supposed to be shorthand for getCustomData and setCustomData which are also defined?
Edit: The analogous code in the C++ controller says that these have been deprecated since 2018, and that they were an alternative alias for the CustomData functions. Not sure whether to remove them, or still keep them, perhaps with a deprecation notice?
controller.py seems to have a fairly convoluted method (inherited from the C++ controller) of maintaining a full list of robot devices. The main useful purpose that this seems to be intended to serve is that it would cache devices so that repeated calls to robot.getDevice or robot.getDeviceFromIndex with the same argument would quickly yield the very same Python object, rather than more slowly returning a new object representing the same device node in the simulation. I'm not fully convinced that this code will even work correctly and quickly across all possible circumstances, and I'm not sure whether I should be building something like this into our new Python controller?
This code seems to assume that device tags and device indices are interchangeable. Is that a safe assumption? I previously had been treating device tags much like constants, where I would just take whatever value Webots currently gives me, but couldn't assume anything more than that it would be unique to the device in question. So, e.g., for all I know, device tags might someday correspond to some sort of node tag that will range to much higher values than the highest device index. If device tags just are indices, then it's mysterious why there would be a robot_get_device_tag_from_index function, since it would always return its argument (or perhaps would occasionally return 0 for out-of-bounds args). If it isn't a safe assumption that tags will never be higher than indices, then the current C++ and python controllers are potentially bugged because they use tags as indices in their list of devices. (Fixing this would probably requiring switching the cache to being something more like a Python dict, which can map arbitrary tags to cached values.)
A comment in the code says that deleted devices will stay on the C API list of devices, and that newly added devices will always expand the C API device count. Hopefully that's right?
When a new device has been added to the robot since the controller last updated its list of devices, this code then fully reconstructs its list of devices, constructing new Python objects for each of them, which seems inefficient if (per the above assumption) the only possible changes would be to the tail end of the list, and also seems to violate the presumed goal of using this cache to avoid creating duplicate objects to represent the same device node. Perhaps some sort of complete refresh like this is needed to keep up with other changes that might have occurred to some of the earlier devices (??), but if that's why the full refresh is being done, then the trigger for doing this probably shouldn't be the addition of new devices, but instead whatever changes in old devices required the full refresh.
I'm guessing that this might have been a crude overkill solution to the bug where devices that were dynamically added to a robot mid-simulation couldn't be used without a controller restart? It looks to me like there could have been a much more surgical solution to that problem that wouldn't have created nearly as many redundant objects. E.g., this should probably keep the old entries from the list rather than recreating all of them. That's easy in Python with lists that lengthen themselves on demand -- perhaps less easy in C++ if lengthening the list requires allocating a larger memory block and copying over whatever old values you want to keep.
So, anyway, I'm trying to decide how much of this to keep in our new Python controller. (Q1) should we expect getDevice functions will be called repeatedly on the same device often enough for it to be worth caching the results? If we do cache them, then it might not be terribly inefficient for people to just keep saying Robot.Motor('left motor') or Device[3] rather than coming up with their own local name for this device. (Q2) How strong of a link can I presume between tags and indices? (Q3) What sorts of changes would a caching system need to beware of, e.g., can deleting devices change other devices' indices, and must I keep slowly re-looking up device names to allow for the strange possibility that a supervisor might have swapped names of devices?
Hrm... Some sort of caching does make some sense for motors/brakes/positionsensors which each have methods for returning their compatriots in the same joint, and it would probably be preferable to have repeated calls to these keep returning the same object rather than spawning new ones...
Edit: Okay, I set up Device.__new__ to use a caching system to avoid constructing duplicate devices, so Device(name) and Device(index) will return an existing device when possible, and otherwise construct a new device of the appropriate subclass. (This is also inherited by subclasses, so e.g. Motor(name) works too.) This system follows the assumption of the C++/swig controllers that indices are equivalent to tags. If that assumption is false (??), my code will need to be changed, but so too will the C++ controller. I also assumed that it doesn't matter what order devices are accessed in, so I just populate the cache as devices are requested, rather than prepopulating it (and then inefficiently fully repopulating it whenever new devices are added) the way the C++/swig controllers did. I suppose it's imaginable that tags are equivalent to indices only in the case where the devices are accessed in index-order (??), in which case my code may need to be changed.
Okay, I think there is one fairly small sort of backwards-incompatibility that I may want to insist upon.
Traditionally, controller.py stored a bunch of NodeType constants (like WB_NODE_ACCELEROMETER) as simple attributes of the Node class (e.g. Node.ACCELEROMETER). In contrast, it is essential to my proxy scene tree that supervisors be able to use (dynamically-generated) attributes of Nodes to refer to descendants in the scene tree, so e.g. ROOT.ROBOT1.LEFT_HAND.ACCELEROMETER should be an easy way of retrieving the node with DEF-name ACCELEROMETER descended from the node with DEF-name LEFT_HAND, descended from ROBOT1 descended from the root of the scene tree. So, in this case, I want LEFT_HAND.ACCELEROMETER to return a particular accelerometer node, not the NodeType WB_NODE_ACCELEROMETER which is what it traditionally would have returned.
Such namespace collisions could be avoided if people were careful to refrain from using DEF-names that are also existing NodeType constant names. However, it is conventional to use all-caps for both DEF-names and for constants, and it is fairly intuitive to give nodes DEF-names that reflect their nodetype, so it seems like this sort of restriction would be annoying and often violated.
Setting aside issues of backwards compatibility, the solution I would want is to have users not use NodeType constants at all, and instead always deal with either Python object classes (like Accelerometer) or strings (like the supervisor function node.getTypeName returns). This would do away with the need to have NodeType constants and the potential name-space collisions that they might have caused.
But that leaves us with the question of what to do about backwards compatibility with old code that had tried to use node.ACCELEROMETER to get a constant that it could compare with the output of node.getType or device.getNodeType. In the case where an ordinary non-supervisor controller tries to access this, I can probably just return the new Python equivalent for this constant as it would be returned by device.getNodeType, the Python Device subclass Accelerometer, along with a deprecation notice. This should fully handle backwards compatibility for ordinary non-supervisor controllers, which have no access to Node instances, and whose only points of contact with NodeTypes were Device.getNodeType and the various Node.CONSTANTS. For supervisor controllers, I might be able to do something similar, though I'll need to think about how this would interact with cases where they meant to be enquiring whether any descendant had this DEF-name, and so would have expected a None or error response, rather than an 'Accelerometer' response. But hopefully most people who write supervisors will either have just used the more convenient getTypeName anyway or will be clever enough to sort out whatever small backwards-incompatibility there is.
Edit: For now, device.getNodeType returns the Python class of the device (just like vanilla python type(device) ), node.getType returns a string version of the node's type (just like node.getTypeName), and the various Node.CONSTANTs return new NodeType objects with overloaded equality comparisons that make them yield the same results as before when compared to either of those outputs. This will provide backwards compatibility while clearing the way to eventually phase out the NodeType constants entirely. So the only lingering issue is how to resolve potential namespace collisions with the proxy scene tree for supervisor controllers. I think it'll probably work well enough to resolve such ambiguities in favor of scene tree nodes (which is how my proxy scene tree always has worked), and to generate a deprecation warning if old nodetype constants end up being accessed.
Many questions...
First of all, I believe we should simply forget about backwards compatibility. I am sure we are going to break it anyhow, so let's break it one for good. We will simply have a guide explaining how to upgrade your old Python code to the new Python API. And that should be pretty straightforward.
For getting a device, I believe we should therefore have only one way to do it and using the class constructor is the most pythonic and simple way to achieve this. And the code should be easy to write, read and maintain, e.g.:
import webots
camera = webots.Camera('my camera')
or:
import webots as wb
camera = wb.Camera('my camera')
or:
from webots import Robot, Camera
camera = Camera('my camera')
Also, you can safely abandon any function or object that is not documented or declared as obsolete.
(Q1) should we expect getDevice functions will be called repeatedly on the same device often enough for it to be worth caching the results?
No, since we will use object instantiation, this is not relevant.
(Q2) How strong of a link can I presume between tags and indices?
See the source code of the libController (C).
(Q3) What sorts of changes would a caching system need to beware of, e.g., can deleting devices change other devices' indices, and must I keep slowly re-looking up device names to allow for the strange possibility that a supervisor might have swapped names of devices?
Again the answer lies in the libController, but I wouldn't implement any caching for now until we have a real performance problem.
Finally, I like your idea to replace all Node.ACCELEROMETER, Node.ALTIMETER, etc. with class names. This is more pythonic and fixes the name clash problem you mentioned. Also, it should be easy to implement. I will give it a shot.