Initial PR for v0.8 #33

mattuntergassmair · 2019-12-16T19:14:38Z

Some major restructuring of the code base, interface changes and deprecations. For a more detailed list of changes please consult the Changelog section in README.md.

mattuntergassmair · 2019-12-16T19:33:08Z

Closes #9

coveralls · 2019-12-17T02:28:35Z

Coverage decreased (-13.009%) to 37.78% when pulling ba1fb0a on mattuntergassmair:v0.8 into d94d04b on sisl:v0.8.

zsunberg · 2019-12-17T05:28:47Z

README.md

+### v0.8.x
+
+#### Rendering
+ - Clean-up of the rendering interface: there is now only one single rendering function with the signature


Does this mean the user has to construct a RenderModel object explicitly?

Yes, I think the typical usage now is as follows:

rm = RenderModel() update_camera!(rm, MyCamera()) render!(rm, [my_renderables...])

I think it would still be convenient to have one render function that allows to do things in one function call and have the explicit version if one wants more fine tuning.

Yes, RenderModel needs to be constructed explicitly. This is mostly because RenderModel holds the main camera parameters (camera_center, camera_zoom, camera_rotation). This is not by my choice but rather how RenderModel was designed to work. In fact if designing from scratch I would probably not have the RenderModel hold those camera parameters.
So in order to set the initial zoom level (for example), it needs to be passed to the RenderModel. In the previous version, the zoom level was set by first setting the zoom level of a camera and then the camera would go set the zoom level of the rendermodel, but effectively those variables were duplicated in RenderModel and Camera which I was not a big fan of. Would you agree?

On a conceptual level, the camera state really lives inside the RenderModel (again, based on legacy code) and the structs that are called Camera actually describe camera motions over time (like "stay static", "follow a vehicle", "follow the scene"). This is quite subtle but I think the struct names may be a little misleading.

Probably worth providing one or two convenience functions that take care of constructing a RenderModel but for full control I think we need all three steps (RenderModel, update_camera!, render!)

@MaximeBouton added an "easy" render function that just takes a scene and optional overlays. There are still a few open questions there regarding the defaults

I will try it out today.

What would be your suggestion on how to handle the camera if we did not have the RenderModel struct?

for the default render method this is what I had in mind:

""" Takes care of initializing a `RenderModel` and updating the camera. For full control, use `render!(rendermodel, renderables)` instead. """ function render( renderables; camera_zoom::Float64 = 10., camera_center::VecE2 = VecE2(0., 0.), camera_rotation::Float64 = 0., camera_motion::Camera = SceneFollowCamera(), canvas_width::Int=DEFAULT_CANVAS_WIDTH, canvas_height::Int=DEFAULT_CANVAS_HEIGHT, surface::CairoSurface = CairoSVGSurface(IOBuffer(), canvas_width, canvas_height), ) rendermodel = RenderModel(camera_center=camera_center, camera_zoom=camera_zoom, camera_rotation=camera_rotation) update_camera!(rendermodel, camera_motion) render!(rendermodel, renderables) return surface end

no need to go back to something specific that takes scene and roadway as input.

I will try it out today.

What would be your suggestion on how to handle the camera if we did not have the RenderModel struct?

One Idea that crossed my mind would be to have a CameraState object that has the fields zoom, position, rotation. Each Camera then has a reference to a CameraState which it then manipulates. And when we pass in the Camera to the render function, with it comes the CameraState object. In addition to the CameraState, Camera itself would hold other variables that parameterize how it moves over the scene (target_id, n_vehicles in scene, etc).
RenderModel would then only keep track of the rendering instructions essentially (maybe the canvas dimension and canvas surface too, didn't spend much thought on this). RenderModel and CameraState would only be combined in the final render_to_canvas function.
In this way, we would not need to pass in the RenderModel to render but only the Camera.

for the default render method this is what I had in mind:

""" Takes care of initializing a `RenderModel` and updating the camera. For full control, use `render!(rendermodel, renderables)` instead. """ function render( renderables; camera_zoom::Float64 = 10., camera_center::VecE2 = VecE2(0., 0.), camera_rotation::Float64 = 0., camera_motion::Camera = SceneFollowCamera(), canvas_width::Int=DEFAULT_CANVAS_WIDTH, canvas_height::Int=DEFAULT_CANVAS_HEIGHT, surface::CairoSurface = CairoSVGSurface(IOBuffer(), canvas_width, canvas_height), ) rendermodel = RenderModel(camera_center=camera_center, camera_zoom=camera_zoom, camera_rotation=camera_rotation) update_camera!(rendermodel, camera_motion) render!(rendermodel, renderables) return surface end

no need to go back to something specific that takes scene and roadway as input.

I wish this were possible, it was also what I initially started off with. But I currently don't see an easy way of doing this:

SceneFollowCamera and TargetFollowCamera both need access to the scene in order to determine their new position (this is why we had issue SceneFollowCamera in new interface #9 where SceneFollowCamera would not work in the new rendering interface). The best we could do would be to default to StaticCamera but not sure if the convenience is worth what we're giving up.

We need to know what is the roadway so that we can render it before (i.e. below) the scene. If we pass in the roadway with the other renderables, we would have to search through the renderables and identify the roadway by type and render it first, which is pretty ugly.
The only way i see around that would be to give every renderable object a depth parameter which we can query using depth(obj) and then determine rendering order based on that. Still not really the way to go I think :/

So given these two constraints, this "convenience" render function was the best I could think of...

zsunberg · 2019-12-17T05:32:58Z

README.md

+All keyword arguments are optional. Objects of type `Renderable` now no longer have to implement the `render!` function (which is a misleading name). Instead one must implement the `add_renderable!` function which adds the rendering instructions to the `RenderModel`.
+ - Implicit conversions of non-renderable objects (such as `obj::Frame{Entity{S,D,I}}`) via implementations of `Base.convert(Renderable, obj)` are now discouraged. Instead, one can overwrite the `add_renderable!` method for such types. This is done for some very common types.
+ - The new `render!` function now only takes objects which are renderable, i.e. which implement the `add_renderable(rm::RenderModel, obj)` function. There is no longer a distinction between drawing roadways, scenes or overlays. They all need to satisfy the same interface, and they are drawn in the order in which they are passed to the `render!` function. This change decreases the number of available render functions from almost ten to one and should make the control flow more clear.
+ - Additional arguments to `render!` such as `camera` and `car_colors` are no longer supported. Camera effects should be applied before calling `render!` (see section below) and rendering attributes such as colors should be passed in as part of a renderable object.


rendering attributes such as colors should be passed in as part of a renderable object.

yes!

zsunberg · 2019-12-17T05:35:46Z

README.md

+render!(rendermodel::RenderModel, renderables::AbstractVector; canvas_width::Int, canvas_height::Int, surface::CairoSurface))
+```
+All keyword arguments are optional. Objects of type `Renderable` now no longer have to implement the `render!` function (which is a misleading name). Instead one must implement the `add_renderable!` function which adds the rendering instructions to the `RenderModel`.
+ - Implicit conversions of non-renderable objects (such as `obj::Frame{Entity{S,D,I}}`) via implementations of `Base.convert(Renderable, obj)` are now discouraged. Instead, one can overwrite the `add_renderable!` method for such types. This is done for some very common types.


yeah, I agree convert(Renderable, ) was kind of clunky

mattuntergassmair · 2019-12-17T23:32:33Z

src/renderable.jl

+    #  A) only allow rendering with roadway (as a first or second argument), scene without a roadway is not needed
+    #  B) implement a second, separate method?
+    #  C) allow roadway to be Union{Nothing, Roadway}?
+    #  D) more options??


roadway should be rendered before (i.e. below) scene. which of the above options makes most sense?

I don't think the default should be specific to roadway (see my other comment)

mattuntergassmair · 2019-12-17T23:34:08Z

src/renderable.jl

+
+    scene_renderables = []
+    for entity in scene
+        color = RGB(rand(), rand(), rand())  # TODO: random colors?? if not, how to determine id? 


Not sure if random colors is a sensible default here... an alternative would be to color-code by ID, but we'd need to know what is the ego_id. Should we just assume ego_id = 1?

I don't think we need that (see earlier comment)
also I don't think we should assume ego_id = 1 as the id might not even be an int.

Agree on what you say about ego_id == 1, but then if we don't use random colors what are our other options?

mattuntergassmair · 2019-12-17T23:36:04Z

test/runtests.jl

@@ -55,6 +55,8 @@ end
    @test AutoViz._rendermode == :fancy

    @test_deprecated render([roadway, veh1, veh2, veh3])
+    # render(Frame([veh1, veh2, veh3]), camera_zoom=15., camera_center=VecE2(1.,1.), camera_motion=TargetFollowCamera(target_id=1))  # TODO: multiple dispatch not working on update_camera!


update_camera! does not get resolved as expected by multiple dispatch. Ideally I would like to enforce that I is the same in TargetFollowCamera{I} and Frame{Entity{S,D,I}} but this doesn't seem to work. The exact reason is beyond me. Could you have a look @MaximeBouton ?
To reproduce, just run the tests locally and uncomment the above line with TargetFollowCamera

It is a pretty sneaky issue, I am not sure I fully understand, but I think it is related to the fact that Vector{Int64} is not a subtype of Vector{Real}.

I hope this example helps:

julia> scene = Frame([veh1, veh2, veh3]) # this is what gets passed in to the `update_camera` method, note the location of the curly braces, the type of scene is Frame{something abstract} Frame{Entity{VehicleState,D,Int64} where D}(3 entities) julia> typeof(scene) <: Frame true julia> typeof(scene) <: Frame{Entity{S,D,I}} where {S,D,I} # this is false, hence the method error, let's investigate false julia> Frame{Entity{VehicleState,VehicleDef,Int64}} <: Frame{Entity{S,D,I}} where {S,D,I} true julia> Frame{Entity{VehicleState,VehicleDef,Real}} <: Frame{Entity{S,D,I}} where {S,D,I} true julia> (Frame{Entity{VehicleState,VehicleDef,R}} where R) <: Frame{Entity{S,D,I}} where {S,D,I} true julia> (Frame{Entity{VehicleState,VehicleDef,R} where R}) <: Frame{Entity{S,D,I}} where {S,D,I} false

Good catch! I didn't realize the curly braces were in different positions, that must be the reason!!

Ok so the following works (but it breaks things further down the call chain):

function update_camera!(rendermodel::RenderModel, camera::TargetFollowCamera{I}, scene::Frame{Entity{S,D,I} where D}) where {S,I}

I think it's related to the fact that vehicle definition is different in some of these cases, so Frame{Entity{S,D,I}} where {S,D,I} enforces all entities to be the same, while Frame{Entity{S,D} where {S,D}} where I enforces only the id to be consistent while state and def can vary among entities in the frame... Food for thought, I need to digest this :-)

MaximeBouton · 2019-12-18T14:48:34Z

Questions on the camera feature:

Should we get rid of StaticCamera since it does nothing?
Why would update_camera! always need the scene? Would it make more sense to have update_camera!(rm, camera) and the camera object would contain all the information? e.g. TargetFollowCamera would have the entity as part of its field and SceneFollowCamera would have the scene.

mattuntergassmair · 2019-12-18T18:54:29Z

Questions on the camera feature:

* Should we get rid of `StaticCamera` since it does nothing?

* Why would `update_camera!` always need the scene? Would it make more sense to have `update_camera!(rm, camera)` and the camera object would contain all the information? e.g. `TargetFollowCamera` would have the entity as part of its field and `SceneFollowCamera` would have the scene.

Good points! As far as StaticCamera, I agree that we could get rid of it in terms of functionality. The reason I kept it around is because it is a "dummy" camera that satisfies the same interface as other cameras. If we didn't have StaticCamera, but we wanted a camera that "does nothing", we would have to set camera_motion = nothing and then do stuff like if camera_motion === nothing ... else ... end. So my preference was to just have a static camera that follows the same interface but does nothing. But there may be better ways of doing this and we could get rid of StaticCamera alltogether.

As for the update_camera interface, it would be great to only take a rendermodel and camera and have the rest as fields of the camera. Say if target_vehicle or scene were members of TargetFollowCamera and SceneFollowCamera respectively, how would we keep them up to date?
Would we re-initialize a new camera in every time step of the visualization, via cam = SceneFollowCamera(scenes[i])?
If we had a SceneFollowCamera(scene) that we keep around over several time steps, we would need to make sure scene stays up to date.

mattuntergassmair · 2019-12-19T00:41:18Z

The two main points remaining are

make the camera interface nicer
provide a better default function for rendering
These will be addressed in a future PR, merging this one for now

Matthias Untergassmair and others added 9 commits December 1, 2019 04:49

RenderableOverlay wrapper

ea0bb8f

cameras and rendermodel refactored

1485835

transferred cameras and additional renderables from DenseTrafficMerging

f1bd17d

deprecated old rendering functions

8b7a64b

added change log to README

173a08b

independent of LightXML

321f13a

veh not defined

43648bd

fix depwarn

d41e592

make do blocks work

7726c84

mattuntergassmair requested a review from MaximeBouton December 16, 2019 19:14

Merge branch 'v0.8' into v0.8

1c0a81b

fallback implementation for deprecated render functions

ba1fb0a

zsunberg reviewed Dec 17, 2019

View reviewed changes

added helper function for easy rendering

d0aac34

mattuntergassmair commented Dec 17, 2019

View reviewed changes

MaximeBouton mentioned this pull request Dec 19, 2019

Wishlist for v0.8 #32

Open

mattuntergassmair closed this Dec 19, 2019

mattuntergassmair reopened this Dec 19, 2019

mattuntergassmair merged commit fc16c80 into sisl:v0.8 Dec 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial PR for v0.8 #33

Initial PR for v0.8 #33

mattuntergassmair commented Dec 16, 2019

mattuntergassmair commented Dec 16, 2019

coveralls commented Dec 17, 2019

zsunberg Dec 17, 2019

MaximeBouton Dec 17, 2019

mattuntergassmair Dec 17, 2019

mattuntergassmair Dec 17, 2019

mattuntergassmair Dec 17, 2019

mattuntergassmair Dec 17, 2019

MaximeBouton Dec 18, 2019

MaximeBouton Dec 18, 2019

mattuntergassmair Dec 18, 2019 •

edited

mattuntergassmair Dec 18, 2019 •

edited

zsunberg Dec 17, 2019

zsunberg Dec 17, 2019

mattuntergassmair Dec 17, 2019

MaximeBouton Dec 18, 2019

mattuntergassmair Dec 17, 2019

MaximeBouton Dec 18, 2019

mattuntergassmair Dec 18, 2019 •

edited

mattuntergassmair Dec 17, 2019

MaximeBouton Dec 18, 2019

mattuntergassmair Dec 18, 2019

mattuntergassmair Dec 18, 2019 •

edited

MaximeBouton commented Dec 18, 2019

mattuntergassmair commented Dec 18, 2019

mattuntergassmair commented Dec 19, 2019

Initial PR for v0.8 #33

Initial PR for v0.8 #33

Conversation

mattuntergassmair commented Dec 16, 2019

mattuntergassmair commented Dec 16, 2019

coveralls commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattuntergassmair Dec 18, 2019 • edited

Choose a reason for hiding this comment

mattuntergassmair Dec 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattuntergassmair Dec 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattuntergassmair Dec 18, 2019 • edited

Choose a reason for hiding this comment

MaximeBouton commented Dec 18, 2019

mattuntergassmair commented Dec 18, 2019

mattuntergassmair commented Dec 19, 2019

mattuntergassmair Dec 18, 2019 •

edited

mattuntergassmair Dec 18, 2019 •

edited

mattuntergassmair Dec 18, 2019 •

edited

mattuntergassmair Dec 18, 2019 •

edited