Enhanced viewport: zoom out, overlays & tooltips (r53_27127)

[MJP]

Bugreport:

If a vehicle has an invalid order, the game crashes with "Assertion failed at line 2309 of ..srcviewport.cpp: to_tile != INVALID_TILE"

Reproduction:
Open attached savegame
Click "Fast forward the game"
Game crashes because station "St. Michael North" is removed after a few seconds, giving Train 1 an invalid order.

Tested on Win7 x64 with build r23862 and all-in-1.diff applied

Thank you for reporting! It should be fixed in r32.

And r32 comes with a slightly improved rendering for high zoom levels:
(with (top)/without (bottom) "scan surroundings", something the smallmap already does... now in viewport)

scan_surroundings.png (51.18 KiB) Viewed 6897 times

[dadymax]

Thanks for cool patch!
I try to make my first patchset of pathes I like and your patch is awesome! Thanks again. Espectualy for support and update for last trunk

[MJP]

Thanks for cool patch!
I try to make my first patchset of pathes I like and your patch is awesome! Thanks again. Espectualy for support and update for last trunk

Thank you for your kind words, I'm glad you find my patch useful

[MJP]

Just in case someone else than me would like to merge my patches with LeXa2's 32bpp anti-aliased blitter, here is a patch to prevent anim glitches in map mode (tested with the revision g043131c).

[LeXa2]

Just in case someone else than me would like to merge my patches with LeXa2's 32bpp anti-aliased blitter, here is a patch to prevent anim glitches in map mode (tested with the revision g043131c).

It Blitter::SetLine() an extension to the base blitter patch introduced by your patches? I'm worried if I had accidentally forgot to implement a required proc I've been unaware about .
BTW what was the reason you hadn't implemented SetLine as a redirect to SetPixel? Performance issues?
I.e. something like:

Code: Select all

void Blitter::SetLine(void *video, int x, int y, uint8 *colours, uint width)
{
	for (; width > 0; width--, x++, colours++) {
		this->SetPixel(video, x, y, *colours);
	}
}

It'd serve well as base implementation which is compatible with any blitter out there and then it'd be possible to add separate patches to blitters if that would be a desired step to address performance-related issues.

[MJP]

Don't worry, you did not forgot anything (besides http://wiki.openttd.org/Coding_style )

SetLine() is indeed implemented for a performance reason: SetPixel() is not used very often by the game so having a call per pixel is not important. On the other hand, my patch relies heavily on it, so SetLine() helps to reduce the amount of CALL (without it, I'm not happy when I scroll the main viewport fast). The best would be to have SetPixel() inlined but I don't know if it's possible with an object method...

Btw, I like your patch very much

[LeXa2]

Don't worry, you did not forgot anything (besides http://wiki.openttd.org/Coding_style )

I wasn't using it initially when started to hack the first bits of the patch so now it's in slow process of getting in line with the "officially approved code style" which happens with the pieces of code I touch/update with the recent development progress. Not a bit deal though as it is not suitable IMO for right-now inclusion into trunk - there's a lot to optimize and there are some hacks to be removed/addressed so _by_the_way_it_is_designed_ regressions would not be introduced.

SetLine() is indeed implemented for a performance reason: SetPixel() is not used very often by the game so having a call per pixel is not important. On the other hand, my patch relies heavily on it, so SetLine() helps to reduce the amount of CALL (without it, I'm not happy when I scroll the main viewport fast). The best would be to have SetPixel() inlined but I don't know if it's possible with an object method...

Well, bad luck though. Inlining methods could be done but not for virtual procs like SetPixel(). Take a look into 32bpp blitter's Blitter::AdjustBrightness() for an example.
What I'd do for this case is still would have a base implementation in parent class done through SetPixel() and then introduce private implementations in all FINAL blitters classes where it's applicable to have max performance. That way you'd ensure that forward compatibility is always there at the cost of the speed the game would perform with newly introduced blitters + your patch. Yeah, I'm a compatibility-paranoid a bit and I don't think that is a bad thing .

Btw, I like your patch very much

Thanks for a warm words, I really appreciate them. A lot of free time had been spent on polishing this piece of code and I still spend my weekends benchmarking Draw()/Endode() performance, finding bottlenecks and trying various way to increase performance as much as I would be. It's great to hear that the results of this work are not wasted and are really used by someone besides me and a couple of my IRL friends.

[MJP]

What I'd do for this case is still would have a base implementation in parent class done through SetPixel() and then introduce private implementations in all FINAL blitters classes where it's applicable to have max performance. That way you'd ensure that forward compatibility is always there at the cost of the speed the game would perform with newly introduced blitters + your patch. Yeah, I'm a compatibility-paranoid a bit and I don't think that is a bad thing .

I understand your point but the problem is anim blitters are derived from the optimized one. An optimized SetLine() would override the generic method. It would feel odd to let an optimized blitter with a slow method. So as there are not too many blitters, I can provide a specific SetLine() everytime it's needed.

A lot of free time had been spent on polishing this piece of code and I still spend my weekends benchmarking Draw()/Endode() performance, finding bottlenecks and trying various way to increase performance as much as I would be. It's great to hear that the results of this work are not wasted and are really used by someone besides me and a couple of my IRL friends.

I know exactly what you mean

[pi1985]

Patch 01_iterator-prefetch-tile.patch removes declaration of OrthogonalTileIterator, but do not make changes in rest of code. For example, it breaks this:

Code: Select all

CommandCost CmdClearArea(TileIndex tile, DoCommandFlag flags, uint64 p1, uint64 p2, const char *text)
{
...
	TileIterator *iter = HasBit(p2, 0) ? (TileIterator *)new DiagonalTileIterator(tile, p1) : new OrthogonalTileIterator(ta);
...
}

and this:

Code: Select all

class AirportTileIterator : public OrthogonalTileIterator {
private:
	const Station *st; ///< The station the airport is a part of.

public:
	/**
	 * Construct the iterator.
	 * @param ta Area, i.e. begin point and width/height of to-be-iterated area.
	 */
	AirportTileIterator(const Station *st) : OrthogonalTileIterator(st->airport), st(st)
	{
		if (!st->TileBelongsToAirport(this->tile)) ++(*this);
	}

	inline TileIterator& operator ++()
	{
		(*this).OrthogonalTileIterator::operator++();
		while (this->tile != INVALID_TILE && !st->TileBelongsToAirport(this->tile)) {
			(*this).OrthogonalTileIterator::operator++();
		}
		return *this;
	}

	virtual TileIterator *Clone() const
	{
		return new AirportTileIterator(*this);
	}
};

and this:

Code: Select all

#define TILE_AREA_LOOP(var, ta) for (OrthogonalTileIterator var(ta); var != INVALID_TILE; ++var)

I look at all-in-one patch, it make other changes into tilearea_type.h than 01_iterator-prefetch-tile.patch.

[MJP]

Patch 01_iterator-prefetch-tile.patch removes declaration of OrthogonalTileIterator

Weird. I can't see where it is removed (and it should not be).

I look at all-in-one patch, it make other changes into tilearea_type.h than 01_iterator-prefetch-tile.patch.

I'll take a deeper look, forgetting to rebuild the all-in-one diff sounds like me...

Anyway, I have an idea to implement and there should be a r41 soon

[pi1985]

Weird. I can't see where it is removed (and it should not be).

Well, I downloaded bundle again, and it's all ok. Looks like version I applied was outdated (r40, file date: Jan 13).

[KeldorKatarn]

are any of these diffs made against current trunk? I get loads of conflicts with this one still.

[MJP]

are any of these diffs made against current trunk? I get loads of conflicts with this one still.

In "(r40_26246)", 26246 means it's intended for trunk revision 26246.

[MJP]

I'm almost done with the new "vegetation" mode.
It adapts itself to the baseset and loaded NewGRFs.
The transition when you zoom out is much better, as seen on screenshots (opposing a normal view at 8X and a map view at 16X).

[fabca2]

Hi,
That's a great job you performed.

I have a remark regarding one of your patch feature, show route steps which is a great standalone feature.

at feature announcement you got these :

r29 brings the route steps feature, I hope you'll like it

I'd strongly advise you to never mix two basically unrelated features in the same patch, unless you already know that this will never, ever go into trunk.

Michael is totally right, how many very nice patch did grow so much (with many options/feature) that they never went into trunk?

Could you possibly take a look and maybe provide a patch for Cargodist as well?

Don't take this the wrong way, but I dont think there is much incentive to rewrite the patch to support another patch which isn't in trunk...

Now it is.

my advise :
Maybe it's interesting to create a separate forum topic with a separate patch just to add this very nice little feature that can reach trunk much quickly (even if it's probably too late for 1.4)

Not all your patchs can go into trunk at the same time, by splitting you can reach trunk one by one, which is better than nothing (split example : show step, path, tooltips, viewport rendering, viewport cycling, viewport shadow...).

in hope most will be available soon in trunk...

note : I speak only about showing steps, not the path. (which could be organised later). you will have better inclusion chance.

[MJP]

That's a great job you performed.

Thanks!

r29 brings the route steps feature, I hope you'll like it

I'd strongly advise you to never mix two basically unrelated features in the same patch, unless you already know that this will never, ever go into trunk.

Michael is totally right, how many very nice patch did grow so much (with many options/feature) that they never went into trunk?

I write patches to play the game with all of them. That purpose obliges me to
mix them somewhere. Keeping a MQ and each patch separately takes time. While
I enjoy writing new features, I find the maintenance task to be quite boring.

The solution is to put the patches asked for inclusion at the beginning of the
queue. It took me a bit of time but I reorganized the whole MQ so the popular
trunkable patches are easy to get (and in a standalone shape).

r41 is ready, I'll post it tomorrow with renewed illustrations.

Industry tooltip comes first, I cleaned up the code (dedup and stuff). I rewrote
stopovers too: it relies on a grf rather than some hardcoded sprite and I've
added support for multiline (see Central):

multiline.png (24.53 KiB) Viewed 5849 times

The "show vehicle's path" feature is unfinished. As it is now, I let this option
disabled. I wanted an accurate extension of it but it's definitely not trivial.
The difficulty/interest ratio gives a low priority for this task. So unless it's
complete, I put this feature at the end of the queue.

EDIT:
01_tooltip-persistence and 02_industry-tooltips are in good shape and could be trunked.
03_vehicle-route-steps may be too.

[MJP]

On a large map, it can take quite some time to decide of the network's shape and
location. That's why large maps are so interesting. In a cooperative game, this
part relies heavily on communication. So, to ease the explanations, I've added
a plan feature which lets you draw lines with the mouse, focus them, hide them
and eventually share them. For the moment, plans are not included in the save
game file. On the screenshot, you can see 3 lines, P3L2 is red because it is
selected in the plans window (if a plan is selected then all its lines are red).
Patch will come soon.

[Eddi]

would it not be easier to reuse code and GUI from the cargodist overlay?

[MJP]

would it not be easier to reuse code and GUI from the cargodist overlay?

I don't think there is much in common besides GfxDrawLine().

I encountered many unexpected difficulties when I tested a multiplayer game.
Variable text from DoCommandP() is limited to 128 bytes. That's short! So far
for transfering a lot of hexadecimal.

Had to hack my way through packet stuff to upgrade the mtu size limitation to
32k and add binary transfer capability. The tcp stack can handle a bit of
fragmentation. And I have other patches I never tested in multiplayer that will
benefit from this too...

The savegame format is also affected so the plans are transfered when a player
join a game.

I tried to mimic the patch description of the trunk (Add:/Codechange:/Feature:).
And to give a better overview, I've done a dependencies graph.

[pi1985]

This makes crash when no plans made (plans_gui.cpp):

Code: Select all

case WID_PLN_VISIBILITY:
	_current_plan->ToggleVisibilityByAll();
	break;

Another querstion. Is it possible to make stopover mark size larger?

Stopover Size.png (108.58 KiB) Viewed 5621 times