[Marble-devel] Re: Review Request: Dgml Time tag handler

[hjain.itbhu at gmail.com]

> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h, line 334
> > <http://svn.reviewboard.kde.org/r/4988/diff/2/?file=33892#file33892line334>
> >
> >     First and foremost I'd like to keep the internal implementation concept of texturemapping as much as possible out of the model API. 
> >     
> >     Why do you need this call anyways? Does it really have to be public?
> >     
> >     There is paintGlobe already. And you should consider just using that. And if you add functionality then please integrate it with existing code instead of doing copy/paste. Thanks :)
> 
> hjain wrote:
>     setupTextureMapper() maps the projection. In absence of mapTexture(), the texture will be mapped on the projection only by setMapTheme(). Since, we need to change the texture without the theme, we need to connect setupTextureMapper() to timeSet() and dateChanged() signals. But since, setupTextureMapper() requires the projection parameter, I have created mapTexture() which gets the current projection by RTTI and then call setupTextureMapper().
>     
>     This function don't need to be public. I have fixed it.
>     
>     paintGlobe is called very frequently. Changing the texture of projection is not required such frequently. Thus, it is better to have a different slot. :)
>     
>     I don't know how to keep it out of model API :(
>     
>     Thanks :)
>     
>
> 
> Torsten Rahn wrote:
>     No. setTextureMapper doesn't "map" the projection. And setMapTheme() also doesn't have the intention of mapping. 
>     What setMapTheme does is it sets the map theme to a new map theme. While doing so it might initialize a new texturemapper (via setupTextureMapper). This means that the texturemapper is getting fully initialized (slow long loud process that wakes up every one, so you want to avoid calling it).
>     
>     So what you are doing here is _completely_ initializing a new texture mapper object every time the time is set. This doesn't make sense, since you don't create a new texture mapper but just setup the one that was setup already. So why set it up again? :-) That's insanely expensive and obviously is not what you want to do. You just want to trigger that the texturemapper will update its output. Yes, your approach also updates the screen (because d->notifyModelChanged()/modelChanged() is called) after the long process of initialization. But that's just a side product.  
>     
>     From what I can see you just want to emit the signal "modelChanged()" instead of calling mapTexture. That's how it would be done properly. 
>     
>     
>     
>     
>     
>     
>

setTextureMapper only initialize a new texture mapper for current texture. Since, every textureLayer has different texture, there may have multiple texture in one theme. We need to change the texture mapper every time the time is set. Thus, setTextuerMapper is not completely initializing for all texture layers in theme. Imho, setTextureMapper should we called every time the time is set. However, instead of creating a new texture mapper every time, a qhash could be created in MarbleModel which will store texture and texture mapper. setTextureMapper will check if textureMapper is already created it will load it otherwise create a new one.

mapTexture() will not be required anymore.


- hjain


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/4988/#review7061
-----------------------------------------------------------


On 2010-08-17 09:19:43, hjain wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/4988/
> -----------------------------------------------------------
> 
> (Updated 2010-08-17 09:19:43)
> 
> 
> Review request for marble and Bastian Holst.
> 
> 
> Summary
> -------
> 
> This patch merges Temperature(July) and Temperature(Dec) themes into Temperature theme by using dgml time tag. Similar, for Precipitation themes.
> Kindly download the earth folder from http://uploading.com/files/5e9755bd/earth.tar.gz/ and replace it with marble/data/maps/earth folder.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/data/CMakeLists.txt 1162190 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/precip-dec/precip-dec-preview.png UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/precip-dec/precip-dec.dgml 1162190 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/precip-jul/precip-jul.jpg UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/precip/precip-preview.png UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/precip/precip.dgml PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/temp-dec/temp-dec-preview.png UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/temp-dec/temp-dec.dgml 1162190 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/temp-jul/temp-jul.jpg UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/temp/temp-preview.png UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/maps/earth/temp/temp.dgml PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.cpp 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/dgml/DgmlElementDictionary.h 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/dgml/DgmlElementDictionary.cpp 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/dgml/DgmlTimeTagHandler.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/dgml/DgmlTimeTagHandler.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneLayer.h 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneLayer.cpp 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneMap.h 1162190 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneMap.cpp 1162190 
>   /trunk/KDE/kdeedu/marble/src/marble_part.rc 1162190 
> 
> Diff: http://svn.reviewboard.kde.org/r/4988/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> hjain
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20101211/8f099a97/attachment.htm