<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/110194/">http://git.reviewboard.kde.org/r/110194/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 25th, 2013, 6:34 p.m. UTC, <b>Dennis Nienhüser</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A 'ship it' with mixed feelings given that it's a vital part of one last years GSoC projects. However I see your point about code quality and lack of use/maintenance and agree with your conclusion. Wouldn't mind to see another patch to prove point three though :-P
In my opinion we should learn from this and similar GSoC experiences and do organizational changes to prevent it. That's off topic here though.</pre>
</blockquote>
<p>On April 25th, 2013, 6:42 p.m. UTC, <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think that the vector tiles could be added as documents to the GeoDataTreeModel as they become visible and be removed again when they disappear. This could be done by a new layer which determines the set of visible tiles based on the view's bounding box. The biggest challenge I see here is to maintain the lifecycle of the tiles inside the GeoDataTreeModel.</pre>
</blockquote>
<p>On April 26th, 2013, 8:57 a.m. UTC, <b>Thibaut Gridel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hi, you are right that it's a burden to maintain unused code.
I think however that the effort should not be discarded on the base that no theme currently uses it.
Therefore i don't think this removal patch would solve the need or be nice to past working code.
It's also sad that the exercise has not been completed post GSoC.
Possible remedies (no priorities or effort related):
- lets define the work to be made in order to put this on track (Bernard you seem to have ideas there, can we use them and break this into tasks?)
- should the code simply be simplified and/or continued in a separate branch for later inclusion?
- Could the map theme simply be added as an "experimental theme" in the site, so that we can have some exposure for it?</pre>
</blockquote>
<p>On April 26th, 2013, 10:48 a.m. UTC, <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yeah, I agree that it's a pity if we removed that code, particularely because it was a vital part of a last year's GSoC project. However, I don't see much code in the current solution that could be reused in an understandable and clean new implementation. The current code is just so complex and touches so many unrelated classes that I prefer to start from scratch here. In particular, this code gets in my way too often for my taste when working on the texture mapping stuff. Therefore, I prefer it to be removed. Given that this feature is currently unused, I think that user's won't miss it.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I looked into it more closely and discovered that the approach I suggested is actually implemented. After playing with the code I even fixed it to work again with the vectorosm theme. Still, quite a bit of refactoring is required to decouple it more from the texture mapping, but I think it's doable. Therefore I don't suggest to apply this patch any more.</pre>
<br />
<p>- Bernhard</p>
<br />
<p>On April 25th, 2013, 4:31 p.m. UTC, Bernhard Beschow wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Bernhard Beschow.</div>
<p style="color: grey;"><i>Updated April 25, 2013, 4:31 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Reasons:
* lack of avialable map themes, so the VectorTileLayer is currently unused in Marble
* causes high maintainance effort when working on texture rendering stack due to its tight coupling while having very different nature
* could be implemented much easier to provide the same features</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/lib/CMakeLists.txt <span style="color: grey">(534a7a9f221e1d414b27dc28fca2f02986e2e8b8)</span></li>
<li>src/lib/ElevationModel.cpp <span style="color: grey">(d81e58d58229dcd91fa1ca8dfdcfb1385b3e1abe)</span></li>
<li>src/lib/MarbleMap.cpp <span style="color: grey">(14ccd36404e4efb620a9022c5d3dbdb34a1365fb)</span></li>
<li>src/lib/MergedLayerDecorator.h <span style="color: grey">(b436ca39e35fde9b52ff204a3a9f448efc078e6e)</span></li>
<li>src/lib/MergedLayerDecorator.cpp <span style="color: grey">(7e73822c1e2d5eb94617be3a893d98f30e48e4f2)</span></li>
<li>src/lib/StackedTile.h <span style="color: grey">(cf9d5689ac7dc37bf1c853bbddd8b4de46ec06e3)</span></li>
<li>src/lib/StackedTile.cpp <span style="color: grey">(9b449da972913af4a8cf4603a85fc8f7228b7cee)</span></li>
<li>src/lib/StackedTileLoader.h <span style="color: grey">(b4d11a0b5446ba7dc0e82f72a369fe44310ff503)</span></li>
<li>src/lib/StackedTileLoader.cpp <span style="color: grey">(5ecc075164649093e8fefddca29a35028cac9be6)</span></li>
<li>src/lib/StackedTile_p.h <span style="color: grey">(d715e3e001dcc376fdefe1b12f93f306cb1ed27a)</span></li>
<li>src/lib/TextureTile.h <span style="color: grey">(fa80ee163536a547605e51af6384edccb10e0ee7)</span></li>
<li>src/lib/TextureTile.cpp <span style="color: grey">(80af7c64d083581ac6134bac40607dfaac246fdd)</span></li>
<li>src/lib/Tile.h <span style="color: grey">(8d445e8d4bbf971d9448a775c9c4a8fcb813296c)</span></li>
<li>src/lib/Tile.cpp <span style="color: grey">(b07cc5b8ec094aff26495932e1ee2219ad3cefda)</span></li>
<li>src/lib/TileLoader.h <span style="color: grey">(72f5eaeae3234e172c6bcff95d269d0007003559)</span></li>
<li>src/lib/TileLoader.cpp <span style="color: grey">(d459d721857f3894325f0dd1c5a398642aa99627)</span></li>
<li>src/lib/VectorTile.h <span style="color: grey">(cbddd99c1c087b87d6f780af520d548c41e4d54f)</span></li>
<li>src/lib/VectorTile.cpp <span style="color: grey">(7c72ced7dc17b4704f7276e9ef1909f97c0aa7d0)</span></li>
<li>src/lib/VectorTileMapper.h <span style="color: grey">(b6a6e4f04df540f084eaf85ba4e744315a9fbc52)</span></li>
<li>src/lib/VectorTileMapper.cpp <span style="color: grey">(6017f29b76da2f72137bfe770917b10c4a4bb685)</span></li>
<li>src/lib/layers/CMakeLists.txt <span style="color: grey">(4efe657038de29f86dcfde6a3d7b4c10da35a8c9)</span></li>
<li>src/lib/layers/TextureLayer.h <span style="color: grey">(7c5f16c500d4a638efc151c20529ba8dc55b4346)</span></li>
<li>src/lib/layers/TextureLayer.cpp <span style="color: grey">(1fe50d8db4dcfe2b8f833f19bef98ce8970f049d)</span></li>
<li>src/lib/layers/VectorTileLayer.h <span style="color: grey">(97be1493194315f49a849d524f8855785327dc9d)</span></li>
<li>src/lib/layers/VectorTileLayer.cpp <span style="color: grey">(5a5deed6f9a0e48fd5cd4cc5068dc046d1662f10)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110194/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>