Re: [Widelands-dev] [Merge] lp:~nha/widelands/graphics into lp:widelands
Some quick responses: Output: - The log contains "Using GL2/GL4 blit path/terrain rendering path" after the "END GRAPHICS REPORT" line Build errors: - I'll have a look at the glbinding variant. - I ran the debug build which complained about style errors which I then fixed. Guess I was too naive to assume that that includes a comprehensive set of checks... Running, performance, etc: - Yes, MacOSX is unfortunately pretty crappy at keeping up with modern OpenGL. It's really surprising that Apple gets away with that, to be honest. - GLES: All the required extensions are actually part of GLES 3.1, despite the GL4 name - I understand the desire to keep things simple. - The minimap renderer can probably be used without additional extensions. - Terrain rendering and the blit path are trickier. Blitting could use instancing instead of SSBOs, but that's probably more of a step backwards (tiny instances aren't very efficient). Terrain rendering needs the table of terrain types in the vertex and fragment shaders; those should fit into a UBO instead of an SSBO, but that still requires an extra extension. Perhaps old-style uniforms could be used as well? I need to look up the available limits for that - it might be possible, but it might not be. So, I'd give up on the separate blit path without extra extensions. Terrain rendering might be possible, but I need to look into it again. Minimap rendering is probably possible, and is the biggest win anyway. -- https://code.launchpad.net/~nha/widelands/graphics/+merge/314279 Your team Widelands Developers is requested to review the proposed merge of lp:~nha/widelands/graphics into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/use_image_cache into lp:widelands
Review: Needs Fixing I wanted to test this branch on my laptop right now, and I have to say it's a bit of a nightmare, all of which can be roughly summarized as "this is probably not a good idea when VRAM is small". Let me list a number of problems in no particular order: 1) Our driver correctly announces support for textures up to 16384 in size, but when you actually allocate such a texture, it is 1GB large, which is going to fail when VRAM is not that big. So that possibility is something that the code should account for. 2) It seems there are plenty of fun bugs in our driver when textures become gigantic, so on the plus side, this is a great test case for making our driver more robust... 3) When I force max_size to 4096, I no longer hit critical driver bugs (only ones leading to rendering errors, oh well). There is still a problem though because the atlas textures add up to something like 110 million texels, which means that the textures are going to take more than 400MB (okay, maybe slightly under since there are also mask textures), which means that on integrated GPUs, you get texture thrashing. (Mine has a 512MB VRAM reservation, and whether it starts thrashing or not seems to depend on what compiz is currently doing - if I only had a single monitor running Widelands fullscreen it would probably okay, but as is performance is occasionally killed completely.) Even without texture atlases, an integrated GPU is going to swap textures occasionally, but it's going to happen much less because the working set is smaller. With texture atlases, your working set is going to be everything almost all of the time. This sucks. Summary: (1) Trying to reduce the atlas size is more important than you thought. (2) Probably the best quick fix option you have is to check VRAM size (e.g. using GLX_MESA_query_renderer) and add some heuristic to disable texture atlases below a certain VRAM size. It should be possible to override the heuristic via the command line. It's kind of sad that you then have a performance optimization that doesn't help the hardware that needs it most... (3) A longer term fix could be some kind of texture streaming. I actually don't know the best way to do this. -- https://code.launchpad.net/~widelands-dev/widelands/use_image_cache/+merge/282106 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/full_texture_atlas. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/use_image_cache into lp:widelands
VRAM is Video RAM, which in the case of integrated graphics is a part of the physical RAM which is reserved for the GPU. Run `free` to get an idea of how large it is approximately: the total memory shown will be less than the physical amount of RAM, reflecting how much is available to the CPU. In addition, there's the GTT or GART, which is a virtual memory aperture through which the GPU can access system RAM. If not all data fits into VRAM, the GPU can still render from the GTT, but it tends to be slower. The 2048M shown in the dmesg log are probably VRAM + GTT. It's possible that since Widelands doesn't use that much bandwidth, rendering from GTT (typically 1-2GB) would be fine. Please do provide an option to disable the texture atlas, and I'll see what can be done about our driver. -- https://code.launchpad.net/~widelands-dev/widelands/use_image_cache/+merge/282106 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/full_texture_atlas. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/use_image_cache into lp:widelands
This is an impressive improvement! Out of curiosity, how large does the texture
atlas end up being?
I have mostly looked at the actual drawing code (I did notice a merge conflict
marker in world.cc though, and bunnybot probably did, too ;)). As I already
said today on IRC, for desktop OpenGL you should really use instancing. You can
copy the Arguments structure almost 1:1 into a buffer with per-instance data
and use a simple vertex shader to reduce the CPU load. I'd understand if you
don't want separate paths for OpenGL ES 2.0 though.
The next-best thing I noticed is using glMapBuffer to avoid an unnecessary
copy. Right now you fill the vertices_ vector (first "copy"). Then glBufferData
copies the vertex data into the buffer object (second copy). And finally the
vertex data gets uploaded to/pulled by the GPU.
Since you know the number of vertices up-front, it would be better to use
glBufferData(..., NULL, ...) to reserve the required amount of space, then use
glMapBuffer and write the vertex data directly into the buffer object, thus
avoiding the second copy. (This would also apply to
(And now that I've written this, I notice that GLES 2.0 apparently doesn't have
glMapBuffer... *sigh*)
Third, it would almost certainly be beneficial to use an element array. That
would reduce the amount of vertex data you need to write by one third, and
would also reduce the vertex shader load on the GPU by up to one third
(Widelands almost certainly isn't shader bound, but hey... perhaps it saves
some power). The element array can even be completely static - it just needs to
grow as needed during application warm-up.
For the buffer handling, the following is actually best for the kind of
streaming draw that Widelands is doing:
1. Determine a reasonably large buffer size N.
2. Use a single buffer object, initialized with glBufferData(..., N, NULL,
GL_STREAM_DRAW);
3. As you go along drawing a frame, fill the buffer object from front to back.
For all vertex data that you write, use glMapBufferRange to map _exactly_ the
range that you want to write for the next draw call, then unmap, then do the
draw call.
4. When the vertex data for the next draw call wouldn't fit in the remaining
space of the buffer, start from the front, but (and this is VERY important,
I've seen plenty of applications screw this up) add the
GL_MAP_INVALIDATE_BUFFER_BIT.
What happens when you do this is that the driver magically maintains a pool of
underlying buffers, all of size N. Whenever you start from 0 with the
MAP_INVALIDATE_BUFFER_BIT, the driver switches the underlying buffer to one
from the pool that is no longer in-flight. As far as I know, all desktop GL
drivers do this.
Now you see that the patch I sent isn't actually optimal. It would be even
better to do something like this:
GLsizeiptr bytes = items.size() * sizeof(T);
if (bytes > size_) {
glBufferData(GL_ARRAY_BUFFER, bytes, items.data(), GL_STREAM_DRAW);
size_ = bytes;
filled_ = bytes;
} else {
GLbitfield access = GL_MAP_WRITE_BIT;
if (filled_ + bytes > size_) {
filled_ = 0;
access |= GL_MAP_INVALIDATE_BUFFER_BIT;
}
void *map = glMapBufferRange(GL_ARRAY_BUFFER, filled_, bytes, access);
memcpy(map, items.data(), bytes);
glUnmapBuffer(GL_ARRAY_BUFFER);
filled_ = (filled_ + bytes + 3) & ~3; // for the memcpy a larger alignment
might be better; the GPU doesn't really care that much as long as you're dword
aligned
}
This is better because then the underlying buffer is always the same size and
the re-use mechanism can work more efficiently. But then you need to check for
support of GL_ARB_map_buffer_range, and apparently GLES 2.0 doesn't have it.
Also, buffers don't have to be the exact same size to be re-used by the driver,
so the impact is probably not too bad.
And of course, while all of the above should reduce CPU load, I have no idea by
how much.
--
https://code.launchpad.net/~widelands-dev/widelands/use_image_cache/+merge/282106
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/use_image_cache into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~nha/widelands/bug1132473 into lp:widelands
Nicolai Hähnle has proposed merging lp:~nha/widelands/bug1132473 into
lp:widelands.
Commit message:
Fix bug #1132473 and other cleanups in the soldier code
Attacking soldiers no longer get stuck when their location disappears while
they are in battle. Furthermore, replace some polling in the battle code by a
signal mechanism to avoid unnecessary work, and remove some particularly spammy
log messages.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1132473 in widelands: soldier hangs at one point
https://bugs.launchpad.net/widelands/+bug/1132473
For more details, see:
https://code.launchpad.net/~nha/widelands/bug1132473/+merge/153657
--
https://code.launchpad.net/~nha/widelands/bug1132473/+merge/153657
Your team Widelands Developers is requested to review the proposed merge of
lp:~nha/widelands/bug1132473 into lp:widelands.
=== modified file 'src/logic/bob.cc'
--- src/logic/bob.cc 2013-03-03 19:46:00 +
+++ src/logic/bob.cc 2013-03-16 12:44:24 +
@@ -1040,7 +1040,7 @@
molog(* svar1: %s\n, m_stack[i].svar1.c_str());
molog(* coords: (%i, %i)\n, m_stack[i].coords.x, m_stack[i].coords.y);
- molog(* diranims:, m_stack[i].diranims);
+ molog(* diranims:);
for (Direction dir = FIRST_DIRECTION; dir = LAST_DIRECTION; ++dir) {
molog( %d, m_stack[i].diranims.get_animation(dir));
}
=== modified file 'src/logic/soldier.cc'
--- src/logic/soldier.cc 2013-03-03 19:46:00 +
+++ src/logic/soldier.cc 2013-03-16 12:44:24 +
@@ -292,7 +292,6 @@
run = m_die_e_name[i];
}
- log( get %s\n, run.c_str());
return get_animation(run.c_str());
}
@@ -832,11 +831,10 @@
return start_task_idle(game, get_animation(idle), -1);
}
- PlayerImmovable * const location = get_location(game);
- BaseImmovable * const imm = game.map()[get_position()].get_immovable();
+ upcast(Building, location, get_location(game));
upcast(Building, enemy, state.objvar1.get(game));
- if (imm == location) {
+ if (location get_position() == location-get_position()) {
if (!enemy) {
molog([attack] returned home\n);
return pop_task(game);
@@ -847,12 +845,13 @@
if (m_battle)
return start_task_battle(game);
- if (signal == blocked)
+ if (signal == blocked) {
// Wait before we try again. Note that this must come *after*
// we check for a battle
// Note that we *should* be woken via sendSpaceSignals,
// so the timeout is just an additional safety net.
return start_task_idle(game, get_animation(idle), 5000);
+ }
if (!location) {
molog([attack] our location disappeared during a battle\n);
@@ -915,7 +914,7 @@
}
}
Flag baseflag = location-base_flag();
- if (imm == baseflag)
+ if (get_position() == baseflag.get_position())
return
start_task_move
(game,
@@ -938,7 +937,7 @@
// At this point, we know that the enemy building still stands,
// and that we're outside in the plains.
- if (imm != enemy-base_flag()) {
+ if (get_position() != enemy-base_flag().get_position()) {
if
(start_task_movepath
(game,
@@ -1414,6 +1413,7 @@
return skip_act(); // we will get a signal via setBattle()
} else {
if (m_combat_walking != CD_COMBAT_E) {
+opponent.send_signal(game, wakeup);
return start_task_move_in_battle(game, CD_WALK_E);
}
}
@@ -1495,7 +1495,6 @@
}
}
} else {
-
assert(opponent.get_position() == get_position());
assert(m_battle == opponent.getBattle());
@@ -1503,22 +1502,21 @@
molog
([battle]: Opponent '%d' is walking, sleeping\n,
opponent.serial());
-return start_task_idle(game, descr().get_animation(idle), 100);
+// We should be woken up by our opponent, but add a timeout anyway for robustness
+return start_task_idle(game, descr().get_animation(idle), 5000);
}
if (m_battle-first()-serial() == serial()) {
-molog([battle]: I am first: '%d'\n, m_combat_walking);
if (m_combat_walking != CD_COMBAT_W) {
- start_task_move_in_battle(game, CD_WALK_W);
- return;
+ molog([battle]: Moving west\n);
+ opponent.send_signal(game, wakeup);
+ return start_task_move_in_battle(game, CD_WALK_W);
}
- }
-
- if (m_battle-second()-serial() == serial()) {
-molog([battle]: I am second: '%d'\n, m_combat_walking);
+ } else {
if (m_combat_walking != CD_COMBAT_E) {
- start_task_move_in_battle(game, CD_WALK_E);
- return;
+ molog([battle]: Moving east\n);
+ opponent.send_signal(game, wakeup);
+ return start_task_move_in_battle(game, CD_WALK_E);
}
}
}
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~csirkeee/widelands/memory-leaks-2 into lp:widelands
Thank you for looking into this, that's very much appreciated! Some comments: 1. In logic/tribe.cc, the m_compatibility_wares: Are the explicit std::string casts really needed? 2. Good catch on the FileSystem handling. However, I would say that the underlying issue is not fixed yet: The interface is badly designed, it is very easy to shoot oneself in the foot with it. Returning a reference to a heap-allocated object has always been bad style, and it is particularly bad in modern C++. I think an even better fix would be to change the interface of FileSystem in a consistent way so that those functions (Make/CreateSubFileSystem) return a boost::shared_ptrFileSystem instead of a reference. In that way, the problem is fixed automatically where FileSystems are used, instead of putting the burden on each call site to pay proper attention. -- https://code.launchpad.net/~csirkeee/widelands/memory-leaks-2/+merge/150290 Your team Widelands Developers is requested to review the proposed merge of lp:~csirkeee/widelands/memory-leaks-2 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug1132466 into lp:widelands
Thank you. However, I just realized myself that the eviction still has a problem that I would like to fix first: When evicting a worker that is in the middle of walking, the reset_tasks() approach that is being taken right now causes the worker to be warped. -- https://code.launchpad.net/~widelands-dev/widelands/bug1132466/+merge/150244 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug1132466. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~csirkeee/widelands/memory-leaks-2 into lp:widelands
Hmm. It still seems somehow not bold enough. The overhead of shared_ptrs wouldn't really matter, and we'd be safer down the line. I realize you wanted to just do some quick fixes (and they do look fine to me - so before we let this branch rot, we should just go ahead and merge it, after all, the perfect is the enemy of the good), but why not just go to the root while we're at it? It does seem to be the right thing to do, at least to me. -- https://code.launchpad.net/~csirkeee/widelands/memory-leaks-2/+merge/150290 Your team Widelands Developers is requested to review the proposed merge of lp:~csirkeee/widelands/memory-leaks-2 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug1132466 into lp:widelands
This fixes the warping issue, and seems to be the correct of doing it. -- https://code.launchpad.net/~widelands-dev/widelands/bug1132466/+merge/150244 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug1132466. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug1132466 into lp:widelands
Nicolai Hähnle has proposed merging lp:~widelands-dev/widelands/bug1132466 into
lp:widelands.
Commit message:
Fix bug #1132466 and a potential cheat related to evicting soldiers
The worker leavebuilding task now deals more robustly with situations in which
the evicted worker is currently outside of the building.
Furthermore, it is no longer possible to evict soldiers whose current position
on the map is not their assigned building (i.e., their location). Note that
this was not possible with the standard user interface anyway, but a cheater
could have modified her version of the game such that an eviction player
command would be sent automatically for her soldiers when they reach low HP
during a battle. Such soldiers would have immediately stopped battle and
returned home.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1132466 in widelands: Evicted workers will become stuck if the are away
from home and the building is not conencted to the road network
https://bugs.launchpad.net/widelands/+bug/1132466
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug1132466/+merge/150244
--
https://code.launchpad.net/~widelands-dev/widelands/bug1132466/+merge/150244
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug1132466 into lp:widelands.
=== modified file 'src/logic/playercommand.cc'
--- src/logic/playercommand.cc 2013-02-04 21:52:38 +
+++ src/logic/playercommand.cc 2013-02-24 21:30:28 +
@@ -638,8 +638,14 @@
{
upcast(Worker, worker, game.objects().get_object(serial));
if (worker worker-owner().player_number() == sender()) {
- worker-reset_tasks(game);
- worker-start_task_gowarehouse(game);
+ if (upcast(Building, building, worker-get_location(game))) {
+ if (upcast(Soldier, soldier, worker)) {
+if (soldier-get_position() != building-get_position())
+ return;
+ }
+ worker-reset_tasks(game);
+ worker-start_task_gowarehouse(game);
+ }
}
}
=== modified file 'src/logic/worker.cc'
--- src/logic/worker.cc 2013-02-11 18:01:26 +
+++ src/logic/worker.cc 2013-02-24 21:30:28 +
@@ -1441,30 +1441,20 @@
}
}
- // If our location is a building, make sure we're actually in it.
- // If we're a building's worker, and we've just been released from
- // the building, we may be somewhere else entirely (e.g. lumberjack, soldier)
- // or we may be on the building's flag for a fetch_from_flag or dropoff
- // task.
+ // If our location is a building, our position may be somewhere else:
+ // We may be on the building's flag for a fetch_from_flag or dropoff task.
+ // We may also be somewhere else entirely (e.g. lumberjack, soldier).
// Similarly for flags.
- if (dynamic_castBuilding const *(location)) {
- BaseImmovable * const position = map[get_position()].get_immovable();
-
- if (position != location) {
- if (upcast(Flag, flag, position)) {
-location = flag;
-set_location(flag);
- } else
-return set_location(0);
- }
+ if (upcast(Building, building, location)) {
+ if (building-get_position() != get_position())
+ return start_task_leavebuilding(game, true);
} else if (upcast(Flag, flag, location)) {
BaseImmovable * const position = map[get_position()].get_immovable();
if (position != flag) {
if (position == flag-get_building()) {
-upcast(Building, building, position);
-set_location(building);
-location = building;
+location = flag-get_building();
+set_location(location);
} else
return set_location(0);
}
@@ -1998,9 +1988,7 @@
// Always leave buildings in an orderly manner,
// even when no warehouses are left to return to
- if
- (location-get_type() == BUILDING
- get_position() == static_castBuilding *(location)-get_position())
+ if (location-get_type() == BUILDING)
return start_task_leavebuilding(game, true);
if (!get_economy()-warehouses().size()) {
@@ -2384,14 +2372,19 @@
else if (signal.size())
return pop_task(game);
- if (upcast(Building, building, game.map().get_immovable(get_position( {
+ upcast(Building, building, get_location(game));
+ if (!building)
+ return pop_task(game);
+
+ Flag baseflag = building-base_flag();
+
+ if (get_position() == building-get_position()) {
assert(building == state.objvar1.get(game));
-
if (!building-leave_check_and_wait(game, *this))
return skip_act();
if (state.ivar1)
- set_location(building-base_flag());
+ set_location(baseflag);
return
start_task_move
@@ -2399,8 +2392,22 @@
WALK_SE,
descr().get_right_walk_anims(does_carry_ware()),
true);
- } else
- return pop_task(game);
+ } else {
+ const Coords flagpos = baseflag.get_position();
+
+ if (state.ivar1)
+ set_location(baseflag);
+
+ if (get_position() == flagpos)
+ return pop_task(game);
+
+ if (!start_task_movepath(game, flagpos, 0, descr().get_right_walk_anims(does_carry_ware( {
+ molog([leavebuilding]: outside
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug1132466 into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug1132466 into lp:widelands has been updated. Commit Message changed to: Fix bug #1132466 and a potential cheat related to evicting soldiers The worker leavebuilding task now deals more robustly with situations in which the evicted worker is currently outside of the building. Furthermore, it is no longer possible to evict soldiers whose current position on the map is not their assigned building (i.e., their location). Note that this was not possible with the standard user interface anyway, but a cheater could have modified her version of the game such that an eviction player command would be sent automatically for her soldiers when they reach low HP during a battle. Such soldiers would have immediately stopped battle and returned home. For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug1132466/+merge/150244 -- https://code.launchpad.net/~widelands-dev/widelands/bug1132466/+merge/150244 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug1132466 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~nha/widelands/bug1125539 into lp:widelands
Nicolai Hähnle has proposed merging lp:~nha/widelands/bug1125539 into
lp:widelands.
Commit message:
Fix for bug #1125539: Always render the road overlay, even when the player sees
all
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1125539 in widelands: Roads not rendered in road building mode
https://bugs.launchpad.net/widelands/+bug/1125539
For more details, see:
https://code.launchpad.net/~nha/widelands/bug1125539/+merge/149272
--
https://code.launchpad.net/~nha/widelands/bug1125539/+merge/149272
Your team Widelands Developers is requested to review the proposed merge of
lp:~nha/widelands/bug1125539 into lp:widelands.
=== modified file 'src/graphic/render/gamerenderer_gl.cc'
--- src/graphic/render/gamerenderer_gl.cc 2013-02-11 18:01:26 +
+++ src/graphic/render/gamerenderer_gl.cc 2013-02-19 13:25:32 +
@@ -501,13 +501,16 @@
uint8_t GameRendererGL::field_roads(const FCoords coords) const
{
+ uint8_t roads;
+ const Map map = m_egbase-map();
if (m_player !m_player-see_all()) {
- const Map map = m_egbase-map();
const Player::Field pf = m_player-fields()[Map::get_index(coords, map.get_width())];
- return pf.roads | map.overlay_manager().get_road_overlay(coords);
+ roads = pf.roads | map.overlay_manager().get_road_overlay(coords);
} else {
- return coords.field-get_roads();
+ roads = coords.field-get_roads();
}
+ roads |= map.overlay_manager().get_road_overlay(coords);
+ return roads;
}
void GameRendererGL::prepare_roads()
=== modified file 'src/graphic/render/gamerenderer_sdl.cc'
--- src/graphic/render/gamerenderer_sdl.cc 2013-02-10 16:41:12 +
+++ src/graphic/render/gamerenderer_sdl.cc 2013-02-19 13:25:32 +
@@ -151,7 +151,7 @@
f_d_texture = get_terrain_texture(f_pf.terrains.d);
tr_d_texture = get_terrain_texture(tr_pf.terrains.d);
-roads = f_pf.roads | map.overlay_manager().get_road_overlay(f);
+roads = f_pf.roads;
f_brightness = node_brightness
(gametime, f_pf.time_node_last_unseen,
@@ -177,6 +177,7 @@
bl_brightness = bl.field-get_brightness();
br_brightness = br.field-get_brightness();
}
+ roads |= map.overlay_manager().get_road_overlay(f);
Vertex f_vert
(f_posx, posy - f.field-get_height() * HEIGHT_FACTOR,
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug995011 into lp:widelands
Well, this time certainly helped make the code a little neater, if only in a small detail. And it might also encourage people to read each other's code more, which could increase the bus count ;-) I am going to change the const thing. As for the variable declaration + assignment in the if clause: I actually like this pattern because it neatly puts the scope exactly where it makes the most sense. Can you explain what went wrong with refactoring? CheckStepWalkOn used to be necessary because the code searched for buildings. Fields with buildings don't have MOVECAPS_WALK, so the only way the old code could ever have found any buildings at all is by allowing a special case exception on the last step of the search. Now that the code searches for flags, which are walkable by definition, this is no longer necessary. Now that I think about it, this might also fix some weirdness in those already buggy cases when a military sites seals a narrow passage off completely: The findAttackSoldiers code may have decided that an attack in northwestern direction is possible (because it searches from the attackees flag and hits one of the fields of the military sites that seals passage), but when the attack is launched, the soldiers would fail to find a way. The new code guarantees that there is a path for the soldiers to follow. -- https://code.launchpad.net/~widelands-dev/widelands/bug995011/+merge/147776 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug995011. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug995011 into lp:widelands
Nicolai Hähnle has proposed merging lp:~widelands-dev/widelands/bug995011 into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #995011 in widelands: They can attack me but I can't attack them
https://bugs.launchpad.net/widelands/+bug/995011
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug995011/+merge/147776
I'm going to be super-consistent about this now and just put up a merge
proposal even for a small change like this. I'm curious how that plays out as a
process?
--
https://code.launchpad.net/~widelands-dev/widelands/bug995011/+merge/147776
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug995011 into lp:widelands.
=== modified file 'src/logic/findimmovable.cc'
--- src/logic/findimmovable.cc 2013-02-10 19:36:24 +
+++ src/logic/findimmovable.cc 2013-02-11 21:24:31 +
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008-2009 by the Widelands Development Team
+ * Copyright (C) 2008-2013 by the Widelands Development Team
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -24,6 +24,8 @@
#include militarysite.h
#include upcast.h
+#include economy/flag.h
+
namespace Widelands {
struct FindImmovableAlwaysTrueImpl {
@@ -71,4 +73,14 @@
return false;
}
+bool FindFlagOf::accept(const BaseImmovable baseimm) const {
+ if (upcast(const Flag, flag, baseimm)) {
+ if (Building * building = flag-get_building()) {
+ if (finder_.accept(*building))
+return true;
+ }
+ }
+ return false;
}
+
+} // namespace Widelands
=== modified file 'src/logic/findimmovable.h'
--- src/logic/findimmovable.h 2013-02-10 19:36:24 +
+++ src/logic/findimmovable.h 2013-02-11 21:24:31 +
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008-2009 by the Widelands Development Team
+ * Copyright (C) 2008-2013 by the Widelands Development Team
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -131,8 +131,15 @@
const Immovable_Descr descr;
};
-
-
-}
+struct FindFlagOf {
+ FindFlagOf(const FindImmovable finder) : finder_(finder) {}
+
+ bool accept(const BaseImmovable ) const;
+
+ FindImmovable finder_;
+};
+
+
+} // namespace Widelands
#endif
=== modified file 'src/logic/player.cc'
--- src/logic/player.cc 2013-02-10 19:36:24 +
+++ src/logic/player.cc 2013-02-11 21:24:31 +
@@ -797,22 +797,22 @@
soldiers-clear();
Map map = egbase().map();
- std::vectorBaseImmovable * immovables;
+ std::vectorBaseImmovable * flags;
map.find_reachable_immovables_unique
(AreaFCoords(map.get_fcoords(flag.get_position()), 25),
- immovables,
- CheckStepWalkOn(MOVECAPS_WALK, false),
- FindImmovablePlayerMilitarySite(*this));
+ flags,
+ CheckStepDefault(MOVECAPS_WALK),
+ FindFlagOf(FindImmovablePlayerMilitarySite(*this)));
- if (immovables.empty())
+ if (flags.empty())
return 0;
- container_iterate_const(std::vectorBaseImmovable *, immovables, i) {
- const MilitarySite ms =
- ref_castMilitarySite, BaseImmovable(**i.current);
- std::vectorSoldier * const present = ms.presentSoldiers();
- uint32_t const nr_staying = ms.minSoldierCapacity();
+ container_iterate_const(std::vectorBaseImmovable *, flags, i) {
+ const Flag * flag = static_castFlag *(*i.current);
+ const MilitarySite * ms = static_castMilitarySite *(flag-get_building());
+ std::vectorSoldier * const present = ms-presentSoldiers();
+ uint32_t const nr_staying = ms-minSoldierCapacity();
uint32_t const nr_present = present.size();
if (nr_staying nr_present) {
uint32_t const nr_taken =
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~sirver/widelands/autocache into lp:widelands
Nice work! The InMemoryImage is still a bit of a hack, but all the rest is now
much cleaner than it used to be. I like the overall architecture a lot. Some
comments on the details:
1. You use non-Doxygen formatted comments a lot in places where it would be
nicer to have Doxygen comments. (The Doxygen build is overall not in the best
shape, but still...)
2. constants.h: In C++, one is supposed to include cstdint etc. instead of
stdint.h etc. (unless perhaps there are some C++ compilers where those files do
not exist?)
3. editor/editorinteractive.cc: around line 155, the c_str() in immname.c_str()
is not necessary
4. AnimationGfx: Why is the image cache stored again in instance of this class
instead of just accessing it via g_gr like everywhere else?
5. graphic/graphic.h: The comment above the declaration of Graphic is outdated,
maybe it's time to change that ;)
6. Compiling with g++, there are many warnings of the type declaration of XXX
shadows a member of 'this'. I guess you're using clang on Mac OSX which
doesn't report such warnings? Here are some examples:
[ 1%] Building CXX object
src/CMakeFiles/widelands_all.dir/graphic/image_transformations.cc.o
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::TransformedImage::TransformedImage(const string,
const Image, SurfaceCache*)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:287:91:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::ResizedImage::ResizedImage(const string, const
Image, SurfaceCache*, uint16_t, uint16_t)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:319:3:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::GrayedOutImage::GrayedOutImage(const string, const
Image, SurfaceCache*)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:341:89:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::ChangeLuminosityImage::ChangeLuminosityImage(const
string, const Image, SurfaceCache*, float, bool)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:358:3:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::PlayerColoredImage::PlayerColoredImage(const string,
const Image, SurfaceCache*, const RGBColor, const Image)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:381:3:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
[ 3%] Building CXX object
src/CMakeFiles/widelands_all.dir/graphic/render/gl_surface.cc.o
/home/nha/dev/widelands/repo/autocache/src/graphic/render/gl_surface.cc: In
member function ‘virtual void GLSurface::draw_line(int32_t, int32_t, int32_t,
int32_t, const RGBColor, uint8_t)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/render/gl_surface.cc:157:87:
warning: declaration of ‘width’ shadows a member of 'this' [-Wshadow]
--
https://code.launchpad.net/~sirver/widelands/autocache/+merge/147559
Your team Widelands Developers is requested to review the proposed merge of
lp:~sirver/widelands/autocache into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~nha/widelands/opengl into lp:widelands
I adjusted the code based on your comments, except the road rendering which just seemed a bit too awkward to unify. Does get_dither_edge_texture fit with your concept of how the ImageCache is supposed to work? -- https://code.launchpad.net/~nha/widelands/opengl/+merge/147207 Your team Widelands Developers is requested to review the proposed merge of lp:~nha/widelands/opengl into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] Changing buildcaps to prevent blockage
Hi everyone, as you know, there are a number of really old bugs in our tracker about how certain things can cause total blockage, especially building big buildings. Think https://bugs.launchpad.net/widelands/+bug/566970 In that bug report, the discussion led to a solution that changes buildcaps so that buildings can only be built if the building does not cause the fields surrounding it to become disconnected. I still believe that this is the sanest solution, but it does have the potential to break some maps that use a lot of acid terrain. Is Widelands right now in a stage of the development cycle where such breakage would be okay, or should such a change be postponed until after a future release? Cheers, Nicolai -- Lerne, wie die Welt wirklich ist, aber vergiss niemals, wie sie sein sollte. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~nha/widelands/opengl into lp:widelands
Nicolai! The code compiles and runs great on later Mac OS X - I cannot check with old OS X as my wife took the laptop with her to work. The speedup is quite tremendous: on 100x speed terrain rendering takes less than 15% CPU now on my box. Superb work as always from you. Thank you. As you might have noticed we took up a review kind of process. The specifics are not clear yet - I just learned on my job that this can be very useful and improve the code significantly. So here are some comments: - the diff here on the site is borked (it reports merge conflicts). Can you repropose? I think it updates the diff. Should be fixed now. - adding the intensity parameter to the load() function seems suboptimal. First it is very specific to OpenGL rendering as SDL has no use for it. Secondly it gives the class a second task (namely loading and then 'flattening' the image). How about moving the edge/road loading and ownership out of graphic into the gameview renderer and giving only the GLTextures the intensity parameter. You could even make a conversion (static) method that could convert an RGB to an intensity texture - this would slow startup by a tad, but would keep this stuff internal to the (gl) terrain rendering - I feel that is a cleaner design. Loading the edge.png texture in GameRendererGL does make sense, though then it would probably be best to make GameRendererGL a singleton to avoid loading the texture multiple times. Or is your new image cache meant to also cache textures provided by some other part of the code, so that multiple instances of GameRendererGL would be using the same texture? - Some of the functions (e.g. drawobject(), but also others) are very long and repetitive? Can you find some seams to offload stuff into private or even better static methods/functions? I would not feel comfi to refactor anything in those methods as they stand now :) I'll have to think about it. The problem is that a lot of that stuff is inherently a two-pass approach. - and should be treated as read-only by derived classes: You made two pointers const, why not the third as well? The renderer is not supposed to modify egbase or player, but it _is_ supposed to modify the RenderTarget. How about not making those protected members, but instead combine them into a struct RenderState or so and pass this down to the functions doing the work? Your call. - RenderTarget dst: I prefer to pass a mutable object by pointer. The reason is that it becomes apparent on the call site if the call might modify the object or not. I realize that you prefer to use reference when an object must be passed (i.e. cannot be NULL), but i feel adding an assert as first line to the function does an equally good job. Your choice of course. So sigra's influence is in decline? I actually agree with you in general, but I believe that most code still passes RenderTargets around by reference and it's better to be consistent. Besides, it's usually pretty obvious that you want RenderTargets to be modified. - I love MapviewPixelFunctions. I will wrap some of those for Lua (there are some values hard coded there). - You pass Point by value regularly - why not use a const reference? Leftovers from the original code, will change. - GameRenderer is a struct, but should be a class. We started to run into trouble with clang complaining about the difference. We now use Class for anything with non trivial methods and struct otherwise. Sorry, you need to type the public: now :) Will change. -- Lerne, wie die Welt wirklich ist, aber vergiss niemals, wie sie sein sollte. https://code.launchpad.net/~nha/widelands/opengl/+merge/147207 Your team Widelands Developers is requested to review the proposed merge of lp:~nha/widelands/opengl into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~aber/widelands/jetAnotherEvictButton into lp:widelands
I suppose this merge proposal should be deleted then. Any objections? -- https://code.launchpad.net/~aber/widelands/jetAnotherEvictButton/+merge/107662 Your team Widelands Developers is requested to review the proposed merge of lp:~aber/widelands/jetAnotherEvictButton into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~nha/widelands/opengl into lp:widelands
Could you please check whether you get the same crashes in current trunk? I remembered having one similar crash when switching between software and OpenGL before my changes, but unfortunately I didn't follow up because I thought there may be something driver-related going on. -- https://code.launchpad.net/~nha/widelands/opengl/+merge/147207 Your team Widelands Developers is requested to review the proposed merge of lp:~nha/widelands/opengl into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~nha/widelands/signals into lp:widelands
Note that dynamic linking on Ubuntu 11.04 does work. I don't have access to the other system right now, so I can't reproduce the exact error message. It may just be a problem with the packages on Debian, but it seems that if we default to static linking, we need to at least provide a build-time switch for that. -- https://code.launchpad.net/~nha/widelands/signals/+merge/82925 Your team Widelands Developers is requested to review the proposed merge of lp:~nha/widelands/signals into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~nomeata/widelands/plot-improvements into lp:widelands
A bit late since this has already been merged, however there's a minor nitpick: DiscreteSlider::set_labels should take its argument as a reference, i.e. as a const std::vectorstd::string . That would avoid one unnecessary copying operation of the label vector. -- https://code.launchpad.net/~nomeata/widelands/plot-improvements/+merge/81649 Your team Widelands Developers is requested to review the proposed merge of lp:~nomeata/widelands/plot-improvements into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/win32_glew into lp:widelands
Review: Approve Merged. I changed things around a little so that GLEW is also used on Linux - after all, we'll probably need it there as well eventually. -- https://code.launchpad.net/~widelands-dev/widelands/win32_glew/+merge/50529 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/win32_glew. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~ab-tools/widelands/minimap into lp:widelands
What if, instead of showing a border of the map (which conceptually doesn't exist) you showed a cross that goes through the player's original HQ? Would that be a good solution for those who prefer to have the border? I have to admit that having the map scroll like this can be a little confusing. What if the map didn't scroll, but was always centered on the player's original HQ? -- https://code.launchpad.net/~ab-tools/widelands/minimap/+merge/27998 Your team Widelands Developers is requested to review the proposed merge of lp:~ab-tools/widelands/minimap into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
