add missing playercolor masks and ditch old playercolor code

[Sigra]

Add missing playercolor masks (for example border markers and flags). Ditch the old playercolor code (patch attached). This simplifies the code tremendously.

[Sigra (sigra)]

The file ditch_old_plrclr_code-1.diff was added: patch to clean up after the old player color handling

[Sigra (sigra)]

File Added: ditch_old_plrclr_code-2.diff

[Sigra (sigra)]

The file ditch_old_plrclr_code-2.diff was added: updated patch for revision 3378

[SirVer (sirver)]

Would this affect backwards compatibility? If so, we should target it for build16.

[Kiscsirke (csirkeee)]

As far as I know it wouldn't affect compatibility in any way, but the
atlantean carrier is still using the old type playercolors (I'm fairly
sure nothing else is). So if the animations for that are replaced I
think removing the old playercolor code will make no difference at
all. (The game doesn't save anything about the animations, in my
experimental MNG branch I use an old savegame to always test if the
animations are loaded correctly)

As for adding missing playercolor masks, yea, a lot of buildings and
workers are missing it, but they don't use the old playercolors, they
just display the same for all players.

By the way, Timo, if you're reading this: I guess you made the new
donkey and horsefarm images in Blender? If you did, could you upload
the .blend files to the media bzr? Then I could do playercolors for
the horsefarm (No empire buildings use playercolor thus far, really
there is no flag or anything on them to use it, so not for the
donkeyfarm, but whatever). Thanks!

András

On 12 March 2010 12:31, SirVer <email address hidden> wrote:
> Would this affect backwards compatibility? If so, we should target it
> for build16.
>
> ** Changed in: widelands
>       Status: New => Confirmed
>
> ** Changed in: widelands
>   Importance: Undecided => Medium
>
> --
> add missing playercolor masks and ditch old playercolor code
> https://bugs.launchpad.net/bugs/536076
> You received this bug notification because you are subscribed to
> widelands.
>
> Status in Widelands: Confirmed
>
> Bug description:
> Add missing playercolor masks (for example border markers and flags). Ditch the old playercolor code (patch attached). This simplifies the code tremendously.
>
>
>

[Alexia Death (alexiade)]

the flags might prove difficult to convert. i think it would be cheaper/nicer to recreate them as blender models.

[Alexia Death (alexiade)]

Im working on replacing the flags at the moment.

[Kiscsirke (csirkeee)]

I've started on an atlantean carrier, results so far can be seen here:

http://www.iwstudio.hu/~kiscsirke/emp_carrier1_se_00.png
http://www.iwstudio.hu/~kiscsirke/emp_carrier1_sw_00.png

I thought about a "toga" style because that would be easy to
differentiate on screen from the older models. But now I don't really
know if togas are rather empire or atlantean style. If it's more
empire style we could still make it empire's carrier and make the old
empire one the atlantean. (But empire already has tunics. But in the
campaign Saledus wears a toga :P)

Still, no atlantean workers are made yet, so I could just start
shaping their style with this, it's just that I don't really have a
feel for what their style should be from their backstory (sorry for
whining about this so much :)

Soo if this would be welcome, I could finish him up before release,
and then graphics-wise everything would be ready to ditch old
playercolor code in build-15 already, so it could be easily done after
release.

András (Kiscsirke)

On 31 March 2010 21:01, Samith Sandanayake <email address hidden> wrote:
> ** Changed in: widelands-media
>   Importance: Undecided => Medium
>
> --
> add missing playercolor masks and ditch old playercolor code
> https://bugs.launchpad.net/bugs/536076
> You received this bug notification because you are subscribed to
> widelands.
>
> Status in Widelands: Confirmed
> Status in Widelands media development: Confirmed
>
> Bug description:
> Add missing playercolor masks (for example border markers and flags). Ditch the old playercolor code (patch attached). This simplifies the code tremendously.
>
>
>

[Chuck Wilder (chuckw20)]

Who has seen a REAL Atlantean? (Maybe Plato, but I think he heard about them second hand as it is and he is no longer around to ask.) :-) So, I say why NOT use togas?

Antlantean style is what we say it will be. Using togas for the atlanteans WOULD differentiate them from the imperials if we agree not to use togas on any of the imperials. I just think we should avoid an eclectic combination of styles within a given tribe and keep things consistent. That's not to say we couldn't use togas and tunics for the imperials as both were prevalent in the Roman culture and use yet a different look for the atlanteans (e.g. loincloths?, full robes?). I'm open to other opinions on this of course, but we must agree on SOME style before work can proceed to complete the worker models.

One might even consider a Meso-American style for the Atlanteans (like the Mayans or Incans) as some scholars argue that possible connection. The atlantean buildings are already vaguely reminiscent of the Mayan architecture. A little tweaking of the buildings and it could be a short jump of the imagination to give the player the intended suggestion.

Thoughts?

[Chuck Wilder (chuckw20)]

This also raises a question about race. So far, the models in the game have been represented with Caucasian skin tones. Here is another avenue of differentiation that could be explored.

[Victor Pelt (victor-pelt)]

i would love to have these guys in build 16. i would leave it as is
for build 15. especially because it might involve a lot of tuning like
setting the hotspot, changing the graphics a few times because they
don't quite work, etc etc

they look nice though

On Mon, Apr 5, 2010 at 4:34 AM, Chuck Wilder <email address hidden> wrote:
> This also raises a question about race.  So far, the models in the game
> have been represented with Caucasian skin tones.  Here is another avenue
> of differentiation that could be explored.
>
> --
> add missing playercolor masks and ditch old playercolor code
> https://bugs.launchpad.net/bugs/536076
> You received this bug notification because you are subscribed to
> widelands.
>
> Status in Widelands: Confirmed
> Status in Widelands media development: Confirmed
>
> Bug description:
> Add missing playercolor masks (for example border markers and flags). Ditch the old playercolor code (patch attached). This simplifies the code tremendously.
>
>
>

[SirVer (sirver)]

when I first though about the atlanteans, I imagined them with distinct fantasy like features: a society firmly ruled by the clerics, a fatalistic acceptance of their fate to vanish in the see, real or imagined magic as perceived building block in society, a greenish colored skin and long hair bound to a ponytail as traditional hairstyle. The atlanteans are a rich people in my imagination. I did never think about their cloths though. tunics seem good to me. Maybe barbarians -> cloth, empire -> leader & metals, atlanteans -> tunics? Hope that helps a bit ;).

 Really nice work, but I agree with Dwarik that it has a certain risk to put in build15. I'd suggest leaving it for build16 (which is right around the corner ;) ).

[SirVer (sirver)]

People, what about this bug now? The flags are fixed up now. The border stones are not yet PC ready at the very least. Something else?

[Kiscsirke (csirkeee)]

Well, for frontiers I don't know if anyone has any plans for them, but
making a masked version from the current one would take approximately
5 minutes, if that's needed.

Other that's left is atlantean carrier. For one, if we want to remove
old playercolor code, we could just replace the carrier with the
generic "not done" worker for the time being, it will be modelled
before build16, don't worry about that. But it's not done yet basicly
because we still didn't get enough input from everyone as to what he
should be like.

Sooo now I made a much more direct thread for this at
http://wl.widelands.org/forum/topic/446 , and now I would like to take
this chance and ask everyone who reads this to go there, and state
their opinion. Thank you :) .

Cheers!

András

On 26 April 2010 17:29, SirVer <email address hidden> wrote:
> People, what about this bug now? The flags are fixed up now. The border
> stones are not yet PC ready at the very least. Something else?
>
> --
> add missing playercolor masks and ditch old playercolor code
> https://bugs.launchpad.net/bugs/536076
> You received this bug notification because you are a member of Widelands
> Media Developers, which is subscribed to Widelands media development.
>
> Status in Widelands: Confirmed
> Status in Widelands media development: Confirmed
>
> Bug description:
> Add missing playercolor masks (for example border markers and flags). Ditch the old playercolor code (patch attached). This simplifies the code tremendously.
>
>
>

[Chuck Wilder (chuckw20)]

I hadn't even thought about the frontiers. Although I did ponder SirVer's
comments in the documentation, namely the conf files, regarding the use of
javelins, standards, etc.
I'd be happy to tackle them, tho it might take me longer than "5 minutes".
:D

Andras, thanks for continuing to pursue discussion on the atlanteans. I've
just posted my initial comments.

Cheers!
Chuck

[SirVer (sirver)]

go ahead chuck, but note that the current graphics are already quite distinct, so a redesign is not necessary. Nevertheless, we need Player color versions of those and I guess the original blender files are lost, but I might be mistaken here.

btw, did you commit your latest models into the widelands-media repository?

[Chuck Wilder (chuckw20)]

On Tue, Apr 27, 2010 at 11:18 AM, SirVer <email address hidden> wrote:

> go ahead chuck, but note that the current graphics are already quite
> distinct, so a redesign is not necessary. Nevertheless, we need Player
> color versions of those and I guess the original blender files are lost,
> but I might be mistaken here.
>
> I'll give it a shot, but I don't think I've come across the original
blender models in the media trunk.

> btw, did you commit your latest models into the widelands-media
> repository?
>
Yep! Revision 118

[SirVer (sirver)]

alright, this is ripe now. Does someone feel like ripping out this old code and see what breaks?

[Chuck Wilder (chuckw20)]

On Wed, Apr 28, 2010 at 12:27 PM, SirVer <email address hidden> wrote:

> alright, this is ripe now. Does someone feel like ripping out this old
> code and see what breaks?
>
>
Well, we know the atlantean carrier still uses it at least. :)

[Chuck Wilder (chuckw20)]

I just ran a text search for "plrclr0_" and it was found in only two files.
../src/graphic/animation.cc, and you guessed it
../tribes/atlanteans/carrier/conf

On Wed, Apr 28, 2010 at 12:39 PM, Chuck Wilder <email address hidden> wrote:

>
>
> On Wed, Apr 28, 2010 at 12:27 PM, SirVer <email address hidden> wrote:
>
>> alright, this is ripe now. Does someone feel like ripping out this old
>> code and see what breaks?
>>
>>
> Well, we know the atlantean carrier still uses it at least. :)
>

[Timowi (timo-wingender)]

I will rip out old code in my OpenGL branch. Please do not do this in trunk too.

I actually did not want to reimplement this code to compile with the new code structure for opengl. So after hearing that all graphics with old playercolor code will be replaced before build16 I took out the old loading code already.

[SirVer (sirver)]

I set this to inprogress because it is already fixed in timowis opengl branch. I also assign him.

[Chuck Wilder (chuckw20)]

Fine.

On Mon, May 10, 2010 at 2:27 AM, SirVer <email address hidden> wrote:

> I set this to inprogress because it is already fixed in timowis opengl
> branch. I also assign him.
>
> ** Changed in: widelands
> Status: Triaged => In Progress
>
> ** Changed in: widelands
> Assignee: (unassigned) => Timowi (timo-wingender)
>
> --
> add missing playercolor masks and ditch old playercolor code
> https://bugs.launchpad.net/bugs/536076
> You received this bug notification because you are a member of Widelands
> Media Developers, which is subscribed to Widelands media development.
>
> Status in Widelands: In Progress
> Status in Widelands media development: Fix Committed
>
> Bug description:
> Add missing playercolor masks (for example border markers and flags). Ditch
> the old playercolor code (patch attached). This simplifies the code
> tremendously.
>
>
>

[Sigra (sigra)]

There are still a lot of leftover playercolor=true in the data files (revision 5414). They should be removed (as in the attached patch http://launchpadlibrarian.net/40687961/ditch_old_plrclr_code-2.diff).

[Chuck Wilder (chuckw20)]

It is my understanding that the "playercolor=true" switch setting in a conf file is required to signal the presence of player color mask files for a specific graphic/animation.

The old "plrclr" switch conf file switch has been removed as has the code that supported it to my understanding.

Are you now saying the "playercolor" switch is also no longer required?

[Chuck Wilder (chuckw20)]

@Sigra: Using bzr revno 5414 under Linux, I removed the playercolor switch from the conf file for the Atlantean carrier and it disabled player color processing for that unit.

Please expand on how this implementation should be made.

[SirVer (sirver)]

chuck, your interpretation is correct and this bug is indeed fixed.

[Sigra (sigra)]

> the "playercolor=true" switch setting in a conf file is required to signal the presence of player color mask files for a specific graphic/animation.

Nothing special is required to signal the presence of a file. The filesystem will provide that information.

[Timowi (timo-wingender)]

With the current code playercolor masks are loaded if playercolor=true is in the config file. Without this no mask files are read. I am for keeping it this way. But I can change this if it is wanted that this is decided by the existence of the files only.

All other parts of the old playercolor code are removed from the code.

[Nicolai Hähnle (nha)]

I'm in favor of keeping it this way.

The reason is that it avoids confusion with "ghost files". If you look at Chuck's commits, there are several ones where he changes the FPS at which things are rendered simply because some people end up with different versions of Widelands data files in the search paths. This causes the frame loading code to load files that are not supposed to be there (but end up in the search path by accident). This is very counterintuitive to people.

To avoid similar problems happening with playercolor files, I would prefer to keep the flag. (In fact, given the above, it would make sense to *add* an additional key/value pair indicating the number of frames in an animation. Or to radically change the way the file loading works. Oh well.)

[Chuck Wilder (chuckw20)]

Personally, from a graphician's vantage point, I prefer the current
implementation. The presence/absence of the playercolor key in the conf
file has value in alerting/reminding the graphician as well as the
application to the presumed presence/absence of the mask files.

On Fri, Jun 18, 2010 at 2:54 PM, Nicolai Hähnle <email address hidden> wrote:

> I'm in favor of keeping it this way.
>
> The reason is that it avoids confusion with "ghost files". If you look
> at Chuck's commits, there are several ones where he changes the FPS at
> which things are rendered simply because some people end up with
> different versions of Widelands data files in the search paths. This
> causes the frame loading code to load files that are not supposed to be
> there (but end up in the search path by accident). This is very
> counterintuitive to people.
>
> Yes, and while not everyone maintains multiple versions of Widelands, the
search paths remain a wildcard that should be dealt with. (No pun
intended.) :)

> ...(In fact, given the above, it would make sense
> to *add* an additional key/value pair indicating the number of frames in
> an animation. Or to radically change the way the file loading works. Oh
> well.)
>
> IMHO I think parameterizing the number of frames in an animation is a
bandaid that still would not mitigate the search path vulnerability.

[Sigra (sigra)]

> To avoid similar problems happening with playercolor files, I would prefer to keep the flag. (In fact, given the above, it would make sense to *add* an additional key/value pair indicating the number of frames in an animation. Or to radically change the way the file loading works. Oh well.)

The radical change sounds much more sensible than keeping this kludge and adding another one. Keeping things simple is a good idea. The designer should just have to put the files there and it should Just Work.

With the playercolor key, the code must check that there are indeed no _pc-files when it is false, to make sure that the designer knows that he forgot to set the value to true after adding the files. (Similarly with the number-of-frames key, the code must check that there are no extra frames that the designer might have tried to add, but he forgot to change the value. But maybe this issue would go away with MNG.)

(As usual, providing the same information in different ways requires extra error checking code to make sure that the information is actually synchronized.)

This filesystem code, known to be somewhere between semi-broken and half-working, continues to cause confusion...

[Nicolai Hähnle (nha)]

I think this is really a bikeshed issue. If you want to change all the code so that it only looks for .png-files in the same "physical" directory where the conf-file came from (adding appropriate functions to the Filesystem interface and so on), then by all means go ahead and do it. I don't agree that this is actually a better solution, but I don't care too much either way. I just think that it's a total waste of time that could be used more productively.

[Sigra (sigra)]

> If you want to change all the code so that it only looks for .png-files in the same "physical" directory where the conf-file came from (adding appropriate functions to the Filesystem interface and so on), then by all means go ahead and do it. I don't agree that this is actually a better solution.

I am curious what the advantage is to look for .png-files in other directories than where the conf file is. (I assume that those are what you called "ghost files".) I guess the advantage is just that the existing code happens to do it that way. Often it seems like the Filesystem abstraction complicates things and causes more trouble than it is worth. Maybe a move to PhysicsFS (discussed from time to time) will simplify things.

[Nicolai Hähnle (nha)]

PhysicsFS conceptually does *exactly* what our current system does (unless things have changed since I last looked at it), so it wouldn't actually change anything about this particular problem. There may be other advantage to moving to PhysicsFS, but this is not one of it.

And this is the main advantage of the way things are currently done: As long as you want the ability to overlay multiple directories into a single virtual hierarchy, the simplest thing is to just overlay multiple directories into a single virtual hierarchy. Simple is good.

[Nicolai Hähnle (nha)]

> And this is the main advantage of the way things are currently done: As
> long as you want the ability to overlay multiple directories into a
> single virtual hierarchy, the simplest thing is to just overlay multiple
> directories into a single virtual hierarchy. Simple is good.

I'm starting to believe that this observation should lead to the real solution
of the problem. After all, do we really want to overlay multiple directories
into a single virtual hierarchy?

I would claim that we don't, at least not really. We do want to have a
separate "write" directory for savegames, replays and user-edited maps.
However, I believe the rest of overlaying multiple directories is actually
more of a kludge to deal with the different places the game data can land in
on Unix-like filesystems.

Perhaps if we did a better job of figuring out where the game data is stored
we could simply get rid of those overlays. This would make the overall system
simpler, and it's always good to fix problems by making things simpler.

[SirVer (sirver)]

I think we want virtual hierarchies for maps and maps only. Obviously we could just check the write directory for maps as well so that we wouldn't need hierarchies at all. I feel that there is more important stuff to work on though and this one flag does not hurt.

but mostly I think we are discussing other bugs here than the one originally intended for this report. If someone things we need something to change, please open a new ticket.

[SirVer (sirver)]

Released in build16-rc1