[DONE - REVIEW] Signs #7952

jeremyosborne · Jun 13, 2014

Game flavor. Good for marking dangerous areas (mine fields, see issue #7800) or other things.

Sign construction and furniture def.
- If sign is bashed or deconstructed, delete any cosmetic signage stored in the map square (signs can't do it themselves, they don't they have writing on them).
Writing on sign (added a map<string, string> cosmetics collection to submaps. Maybe I should just have signs be like graffiti and be a single member...).
examine function for signs (reading and writing new text on a sign).
sign writing shows up in info when cursor is over the sign.
sign writing shows up when walking over sign.
signs plus writing defined for mapgen.

Open to suggestions, the above are what I'd like to put in.

Frost-wood · Jun 13, 2014

Good idea, I wanted to add some No-tresspassing signs around the Rivtech facility I'm designing, so that new players don't wander into a death trap.

KA101 · Jun 13, 2014

You're blockin' up the scenery and breakin' my mind! ;-P

Good thinking, looking forward to this.

Rivet-the-Zombie · Jun 13, 2014

I love it! This could be tied into the town names system to show names and populations of towns.

Zireael07 · Jun 13, 2014

Agreed with Rivet.

MattAmmann · Jun 13, 2014

I anticipate that this will open up my eyes (nice branch name) ;)

kevingranade · Jun 13, 2014

For player-writable messages, perhaps overload the grafitti map entity?
That has its own issues, but currently furniture is stateless so there's
nowhere to stash that info.
Sounds neat though.

jeremyosborne · Jun 13, 2014

Glad you guys like music humor. The tesla reference threw me off for just a second.

Kevin, thanks for the warning. I wasn't sure if furniture persisted anything or not. I'll take a look at the graffiti code as a starting point.

jeremyosborne · Jun 15, 2014

About to checkin last piece. Most pieces work, except that I can't confirm map generation of signage works in game, despite how many debugmsg I place in the code. Editmap is a bit of a mystery to me.

Check boxes in the original comments explain most things.

Signs can be built via construction. Recipe is simplistic. This leads to all signs being treated as nailed together wood signs.
Signs built have no writing on them.
Signs can be dynamically changed with spray paint.
Signs, being furniture, store their writing in a variable in the submap.
Sign data does save and load.
Sign data can be configured in mapgen via place_specials (toilet for reference):

"place_specials": [
    { "type": "toilet", "x": 11, "y": 22 },
    { "type": "sign", "x": 10, "y": 22, "signage": "hello cataclysm" }
]

I can confirm that the sign furniture appears in my tests when I use the editmap/mapgen code debug thingy in game. But I can't, no matter what I try, get the writing on the sign to propagate from the temporary submap into the actual game. Ugh.

BevapDin · Jun 15, 2014

BevapDin reviewed Jun 15, 2014

View changes

BevapDin · Jun 15, 2014

src/iexamine.cpp

@@ -1982,6 +1982,47 @@ void iexamine::curtains(player *p, map *m, const int examx, const int examy) {
    }
 }

+void iexamine::sign(player *p, map *m, int examx, int examy)
+{
+    (void)g; //unused


You can remove that line, g once was a parameter to the iexamine functions, but now it's a global variable.

Done in the next check in.

BevapDin · Jun 15, 2014

BevapDin reviewed Jun 15, 2014

View changes

BevapDin · Jun 15, 2014

src/iexamine.cpp

+{
+    (void)g; //unused
+    std::string existing_signage = m->get_signage(examx, examy);
+    bool previous_signage_exists = (bool)existing_signage.size();


How about !existing_signage.empty() instead of a C-style cast?

Done. I'll probably make lots of silly stylistic errors like that.

BevapDin · Jun 15, 2014

BevapDin reviewed Jun 15, 2014

View changes

BevapDin · Jun 15, 2014

src/iexamine.cpp

+
+    // Display existing message, or lack thereof.
+    if (previous_signage_exists) {
+        popup(_(existing_signage.c_str()));


The translation (the _(...)) should be done when the sing is created. Currently I can spray the text "Maximum trait points" onto that sign and when I examine it, the translated text for this is shown (as the _ function translates the text that I have just entered). This of course only works in non-English environment.

Also, the code that displays the sign text in game.cpp does not translate the text at all.

Good point. I removed the translation and just pass through the string the user put in. Should we translate the strings at all? Maybe for strings loaded in the JSON?

BevapDin · Jun 15, 2014

I can confirm that the sign furniture appears in my tests when I use the editmap/mapgen code debug thingy in game. But I can't, no matter what I try, get the writing on the sign to propagate from the temporary submap into the actual game. Ugh.

In my test it did one time appear correctly, but not the other times. And in this case the house was in its original (as from the json data) orientation. I guess you forgot to rotate cosmetics in map::rotate (found in mapgen.cpp).

If I use the debug mapgen to generate the house and rotate it there, the sign text is missing.

jeremyosborne · Jun 16, 2014

BevapDin, i would have never figured out the map::rotate thing. Thank you. Seems to work now by adding in the portage of the signage.

KA101 · Jun 17, 2014

KA101 reviewed Jun 17, 2014

View changes

KA101 · Jun 17, 2014

src/iexamine.cpp

@@ -851,7 +851,6 @@ void iexamine::safe(player *p, map *m, int examx, int examy) {
 }

 void iexamine::bulletin_board(player *p, map *m, int examx, int examy) {
-    (void)g; (void)p; //unused


KA101 is being a bit melodramatic here

Nope. if the function calls for p, you'd best make/fake a use for it somewhere. Otherwise the compiler won't be happy.

And when the compiler isn't happy? builds fail and nobody gets DDA!

I really need to get the same warning level that you have, which I thought the basic build targets did. Which make target do you use for the builds? The presence or lack of the (void)p doesn't throw any warnings or errors in my build, which is:

make NATIVE=osx OSX_MIN=10.7 RELEASE=1 TILES=1 LOCALIZE=0 CLANG=1

Jenkins errors on unused parameters; I imagine Kevin set that to keep our code tidier. I had to make a custom one for mine: -Werror=unused-parameter

KA101 · Jun 17, 2014

Looks good. Now folks'll have to implement it into their buildings, etc. Stand by.

jeremyosborne · Jun 17, 2014

If people report issues, @ me in the messages if you notice before me and I'll fix them (or at least try).

Yep, it's a sign.

b07ae3e

Last checkin. Done for now.

9b3b5aa

jeremyosborne changed the title ~~[WIP] Signs~~ [DONE - REVIEW] Signs Jun 15, 2014

jeremyosborne added some commits Jun 16, 2014

Merge remote-tracking branch 'upstream/master' into i_saw_the_sign

94da5b1

Merge remote-tracking branch 'upstream/master' into i_saw_the_sign

cfa0df1

KA101 self-assigned this Jun 17, 2014

KA101 removed their assignment Jun 17, 2014

Adding the void that was subtracted.

495576a

KA101 self-assigned this Jun 17, 2014

KA101 merged commit 495576a into CleverRaven:master Jun 17, 2014

jeremyosborne deleted the jeremyosborne:i_saw_the_sign branch Jun 17, 2014

Soyweiser referenced this pull request Jul 2, 2016
Open
Make towns have signs to display town or city names. #17482

CleverRaven/Cataclysm-DDA

Join GitHub today

[DONE - REVIEW] Signs #7952

Conversation

jeremyosborne commented Jun 13, 2014

This comment has been minimized.

Frost-wood commented Jun 13, 2014

This comment has been minimized.

KA101 commented Jun 13, 2014

This comment has been minimized.

Rivet-the-Zombie commented Jun 13, 2014

This comment has been minimized.

Zireael07 commented Jun 13, 2014

This comment has been minimized.

MattAmmann commented Jun 13, 2014

This comment has been minimized.

kevingranade commented Jun 13, 2014

This comment has been minimized.

jeremyosborne commented Jun 13, 2014

jeremyosborne added some commits Jun 14, 2014

This comment has been minimized.

jeremyosborne commented Jun 15, 2014

jeremyosborne changed the title [WIP] Signs [DONE - REVIEW] Signs Jun 15, 2014

BevapDin reviewed Jun 15, 2014 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Jun 15, 2014 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Jun 15, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

BevapDin commented Jun 15, 2014

jeremyosborne added some commits Jun 16, 2014

This comment has been minimized.

jeremyosborne commented Jun 16, 2014

jeremyosborne added some commits Jun 16, 2014

KA101 self-assigned this Jun 17, 2014

KA101 reviewed Jun 17, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KA101 removed their assignment Jun 17, 2014

KA101 self-assigned this Jun 17, 2014

This comment has been minimized.

KA101 commented Jun 17, 2014

KA101 merged commit 495576a into CleverRaven:master Jun 17, 2014

jeremyosborne deleted the jeremyosborne:i_saw_the_sign branch Jun 17, 2014

This comment has been minimized.

jeremyosborne commented Jun 17, 2014

Soyweiser referenced this pull request Jul 2, 2016

BevapDin reviewed Jun 15, 2014

View changes

BevapDin reviewed Jun 15, 2014

View changes

BevapDin reviewed Jun 15, 2014

View changes

KA101 reviewed Jun 17, 2014

View changes

KA101 merged commit `495576a` into CleverRaven:master Jun 17, 2014