Core stabilisation

[gchoqueux]

The core isn't stable and many parts are temporary.
The priorities are :

• browse tree generic for all layer, process node (culling) scene and realtime process (RTC, uniform...)
• generic material for multi layer
• solid and stable loop request/rendering
• Debug core, factorization, heritage
• Deployment, prod stack

For the moment the others features aren't priorities and will not be integrated in master branch. it isn't possible to work on a bad base. (may be an exception for #41 )

Thanks

[vpicavet]

Well, priorities in opensource software are based on :

• the amount of work you can put into the development ( and maintainance of your parts)
• Consensus between developers in the community
• PSC decisions in case of conflict

They should be discussed on the mailing list first. Different users have different priorities.
Having unit tests, full use cases also help keep the focus on practical things.

I do not say that your items are not legitimate nor relevant, I just want to remind that this is a collaborative open project and should be managed as such.

[Jeremy-Gaillard]

I've been reading some of the code, and listed a number of improvements:

• c3DEngine should not have this.scene.wait() in its update function. Should it even have an update function? Isn't render enough?
• The update loop is badly implemented. See CPU load super heavy when close to the ground.  #27.
• Still in the update loop: there shouldn't be a switch on the type of layer, it isn't easily maintainable. A possible fix: when adding a layer, pass a function that defines its update mechanic.
• Scene.quadtreerequest is called only for the first layer.
• In BrowseTree and Scene: remove all "quadtree"s from function names. The processes apply (or should apply) to any hierarchical structures (or even any spatial structure).
• ManagerCommands shouldn't be called by the scene but should be a background process in a (or multiple) web worker(s). Since this is a pretty big feature, I think it may be better to wait for the rest of the tasks to be completed.
• Scene.add shouldn't create providers.
• All of BrowseTree #54 still apply

[Jeremy-Gaillard]

Scene is being specialised for the use cases of IGN (setStreetLevelImageryOn, getGlobe). It needs to stay generic, without any assumptions on the nature of the data inside it.
If you really want to keep these kind of functions, you should create a new specialised class that inherits from scene.

[gchoqueux]

Thank you for those remarks

1. I don't like this.scene.wait(), I search for good loop (renderer, request, loader). For the moment the main loop is in c3Engine and the scene updates all nodes. We must see to move the main loop in Scene. The updates nodes needs some mechanisms (culling, request, loader) if subdividing tiles was possible for each frames so the performances falls. The wait() function updates nodes when the movements camera are complete, to avoid a bottleneck with unnecessary queries.
2. The update loop changed, there isn't infinity loop and bounding box are more accurate. @nosy-b can you check CPU load super heavy when close to the ground.  #27 ?
3. Yes, update loop needs to be generic (enhancement generic mechanics update)
4. quadtreerequest is temporary (enhancement generic mechanics update)
5. 'quadtree's from function names are temporary ( enhancement generic mechanics update)
6. Put ManagerCommands a background process may be a good idea.
   7.Scene.add shouldn't create providers -> indeed, we must move the create providers
7. BrowseTree #54 see (3,4 and 5)

[Jeremy-Gaillard]

If it is ok with you, I will be doing 7 + some of the work on #54. The goal is to modify scene.add to be able to define a nodeProcess for each layer. This will affect scene, browseTree and nodeProcess files.

[gchoqueux]

I work on all points, it's difficult to share the points. We must reflect on the distribution.

[vpicavet]

Distribution should be easier if : 

• GH issues are created for each point, defining the perimeter of planned action and assigning people
• Work on each issue is made in a specific public branch
• Code reviews are done on these branches
• Branches are merged to master only after review
• Branches are cleaned up before merge : rebase and squash unnecessary commits

This is important to allow for good collaboration, even if it adds some overhead to the development. It will ensure more quality, and provide visibility on the advances and development process, which is an extremely important point.

[nosy-b]

Agreed,
and as we talked with @dgricci we should provide some unit tests to validate the branch before the merging.
Not an easy task with our kind of application but using the API we could for example verify that positionning the camera at position P, it should launch the urlRequests {url} and check the validity. Any expertise on that is welcome :)

[Jeremy-Gaillard]

I opened an issue related to my previous comment: #67.

[nosy-b]

(Street level imagery is a work in progress, don't be afraid to break it when changing core functions
And yes - scene should be completely generic

• Update Loop (parsing trees, culling...) doesn't have to be totally correlated with Render function. Same for commandmanager, should happen as asynchronous background process)

[Mkonini]

Hi all,

Fully agree, we have to implement unit functional tests for testing each function before merging, like check the url of a request or verify that the pointed area is the right one … etc. We are thinking about how to do that, no examples to share yet, maybe in the next weeks.

De : Alexandre Devaux [mailto:notifications@github.com]
Envoyé : mercredi 30 mars 2016 16:38
À : iTowns/itowns2
Objet : Re: [iTowns/itowns2] Core stabilisation (#57)

Agreed,
and as we talked with @dgriccihttps://github.com/dgricci we should provide some unit tests to validate the branch before the merging.
Not an easy task with our kind of application but using the API we could for example verify that positionning the camera at position P, it should launch the urlRequests {url} and check the validity. Any expertise on that is welcome :)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHubhttps://github.com//issues/57#issuecomment-203462887

[gchoqueux]

Issue too generalist and obsolete