Tileset loading refactoring #13151

VlasovVitaly · Aug 1, 2015

This is fix of #12800

After merging all tilesets authors MUST write path in config files strongly relative.

Tested on Linux.

TODO:

Replace all paths in JSON and TILESET values in all tileset.txt files to related values.
Replace all path from json configs to related values
New var in FILENAMES which contain default config name.
New global mapping TILESETS(string, string) which store all tilesets names and their directories with PREFIX support.
Cache TILESETS mapping at options loading time.
Refactoring of get_tile_information() and other function related to tilesets path.
Doxygen documentation.

VlasovVitaly · Aug 1, 2015

Not so sure about that ifdef, but works for me. Linux curses, sdl builds

VlasovVitaly · Aug 1, 2015

Ok. Test this on Linux with and without PREFIX, with make and cmake build.
Right now i am wrinting doxygen and fixing var names and making minor changes. This can be done by separate PR,

All working. So Request for comment and testers. Need tests on Windows (with and without CLion) and MacOSX

chaosvolt · Aug 1, 2015

After merging all tilesets authors MUST write path in config files strongly relative.

If you're relying on all the tileset authors fixing this on their end instead of adding the required tweaks in this PR, things is going to break royally. :V

narc0tiq · Aug 1, 2015

We'll fix the ones we're including, but we can't cleanly allow a legacy
option here (and things were fairly broken before, so keeping that legacy
means keeping known-bad code).

On Sat, Aug 1, 2015 at 6:54 PM Chaosvolt notifications@github.com wrote:

After merging all tilesets authors MUST write path in config files
strongly relative.

If you're relying on all the tileset authors fixing this on their end
instead of adding the required tweaks in this PR, things is going to break
royally. :V

—
Reply to this email directly or view it on GitHub
#13151 (comment)
.

chaosvolt · Aug 1, 2015

Ah, so this will only be a hazard to modders making their own tilesets that haven't been PR'd, then?

narc0tiq · Aug 1, 2015

Yes. You can see the diff on this PR: it updates all the tilesets to use the correct kind of references, right from the first commit (2751bf3).

chaosvolt · Aug 1, 2015

Ah, right. Doh. >.<

DavidKeaton · Aug 1, 2015

DavidKeaton reviewed Aug 1, 2015

View changes

DavidKeaton · Aug 1, 2015

src/options.cpp

+void options_data::add_value( const std::string &lvar, const std::string &lval,
+                              std::string lvalname )
+{
+    static const std::string blank_value( 1, 001 );


you could move this to the top of the source, so the entire source can reference the same one, as you use this more than once! :-)

This piece of code is move on top. Was on bottom of file.

Why exactly did you move the definitions of those options_data functions? They can be "used" once they have been declared (which happens in option.h, which is include at the very top of this file), the place of their definition does not matter at all. Have those functions actually been changed?

chaosvolt · Aug 1, 2015

Hmm. Just realized, I have been finding yet more stuff I oughta add to a tileset update, which I was planning to do when I wasn't juggling half a dozen PRs. Though to avoid breaking stuff, I'll want to wait until after this is merged since it includes edits to tile_config.

Good, gives me more time for spriting. I am going to dread the day I stop putting off making sprites for the animatronic and dinosaur mods. XP

VlasovVitaly · Aug 5, 2015

Was merged with upstream.

chaosvolt · Aug 5, 2015

Doh? Such confuse, very derp. :V

EDIT: You sure any of this has been merged? Doesn't look like it.

Zireael07 · Aug 5, 2015

Not merged into master, master merged into it to avoid any conflicts.

chaosvolt · Aug 5, 2015

Ah right.

BevapDin · Aug 7, 2015

BevapDin reviewed Aug 7, 2015

View changes

BevapDin · Aug 7, 2015

src/options.cpp

@@ -456,7 +500,81 @@ bool cOpt::operator!=(const std::string sCompare) const
    return !(*this == sCompare);
 }

-void initOptions()
+/** Fill TILESETS mapping with values.
+ * Scans all directores in gfx directory for file named "tileset.txt".


Actually it scans the FILENAMES["gfxdir"] directory for files names FILENAMES["tileset-conf"]. Both entries of the FILENAMES map may contain any string, not just "gfx" and "tileset.txt".

Yeah. Trying to make this description shorter. Guess it's better to fix that message.

BevapDin · Aug 7, 2015

BevapDin reviewed Aug 7, 2015

View changes

BevapDin · Aug 7, 2015

src/options.cpp

+
+        fin.open( file.c_str() );
+        if(!fin.is_open()) {
+            fin.close();


It's not open, why close it?

This is old behavior. Really i don't know why close() is calling. Will be fixed.

BevapDin · Aug 7, 2015

BevapDin reviewed Aug 7, 2015

View changes

BevapDin · Aug 7, 2015

src/options.cpp

+
+    if(tileset_names == "") {
+        optionNames["deon"] = _("Deon's");          // more standards
+        optionNames["hoder"] = _("Hoder's");


Hmmm, those are the only translated tileset names. The other tileset go untranslated. Either they should not be translated at all (they are names anyway), or the lang/extract_json_strings.py or lang/update_pot.sh need to be changed to extract the tileset names. See also line 554, where untranslated entries are added to optionNames.

Hmm. I have found that several languages have own translation of "Deon's" and "Hoder's" strings.
I will add viewname to extractor in separate PR.

BevapDin · Aug 7, 2015

BevapDin reviewed Aug 7, 2015

View changes

BevapDin · Aug 7, 2015

src/options.cpp

+                if (sOption.find("NAME") != std::string::npos) {
+                    tileset_name = "";
+                    fin >> tileset_name;
+                    if(first_tileset_name) {


Instead of having first_tileset_name (and setting it correctly), why don't you just check tileset_names.empty()? If it's empty, the current tileset is the first and no "," needs to be added.

Yeah. Is't better. Will be fixed.

BevapDin · Aug 7, 2015

BevapDin reviewed Aug 7, 2015

View changes

BevapDin · Aug 7, 2015

src/cata_tiles.h

         */
-        void load_tilejson_from_file(std::ifstream &f, const std::string &imagepath);
+        void load_tilejson_from_file(const std::string tileset_dir, std::ifstream &f, const std::string &image_path);


As you already made it const, it should also be a reference, not a copy. Just like image_path.

Ok. i will fix that.
Where i can read about this difference. Any link?

chaosvolt · Aug 7, 2015

Ah delicious, more things to give me plenty of time to work on my tileset updates. Still only halfway done. XP

VlasovVitaly · Aug 7, 2015

Fixes from @BevapDin will be in separate PR.

So at this moment paths in all new tileset must be related.
@chaosvolt @kevingranade please warn authors in IRC and forum.

chaosvolt · Aug 7, 2015

please warm authors in IRC and forum.

So long as they don't give me the cold shoulder, I guess I'll do so. XP

VlasovVitaly · Aug 7, 2015

:p

chaosvolt · Aug 7, 2015

Posted a thread in Lab forum, hope that's the ideal spot in the forums for said warning. I've never actually mucked about on the IRC, so...

CIB · Aug 8, 2015

Should be fairly easy to write a small script for conversion.. It's JSON, after all, can be read and written in any language.

VlasovVitaly changed the title ~~[WIP] Tileset loading refactoring~~ [RFC] Tileset loading refactoring Aug 1, 2015

VlasovVitaly changed the title ~~[RFC] Tileset loading refactoring~~ Tileset loading refactoring Aug 2, 2015

kevingranade merged commit ca58abb into CleverRaven:master Aug 7, 2015
1 check passed

1 check passed

default
Details

VlasovVitaly referenced this pull request Aug 8, 2015
Merged
Fixups after 13151 #13219

VlasovVitaly deleted the VlasovVitaly:fix-filenames-tiles-loading branch Aug 11, 2015

drbig referenced this pull request Aug 12, 2015
Closed
Explosion effect no longer recognized by tilesets #13250

CleverRaven/Cataclysm-DDA

Join GitHub today

Tileset loading refactoring #13151

Conversation

VlasovVitaly commented Aug 1, 2015

VlasovVitaly added some commits Aug 1, 2015

This comment has been minimized.

VlasovVitaly commented on src/path_info.cpp in e7ff724 Aug 1, 2015

VlasovVitaly added some commits Aug 1, 2015

This comment has been minimized.

VlasovVitaly commented Aug 1, 2015

VlasovVitaly changed the title [WIP] Tileset loading refactoring [RFC] Tileset loading refactoring Aug 1, 2015

This comment has been minimized.

chaosvolt commented Aug 1, 2015

This comment has been minimized.

narc0tiq commented Aug 1, 2015

This comment has been minimized.

chaosvolt commented Aug 1, 2015

This comment has been minimized.

narc0tiq commented Aug 1, 2015

This comment has been minimized.

chaosvolt commented Aug 1, 2015

DavidKeaton reviewed Aug 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

chaosvolt commented Aug 1, 2015

VlasovVitaly changed the title [RFC] Tileset loading refactoring Tileset loading refactoring Aug 2, 2015

VlasovVitaly added some commits Aug 2, 2015

This comment has been minimized.

VlasovVitaly commented Aug 5, 2015

This comment has been minimized.

chaosvolt commented Aug 5, 2015

This comment has been minimized.

Zireael07 commented Aug 5, 2015

This comment has been minimized.

chaosvolt commented Aug 5, 2015

BevapDin reviewed Aug 7, 2015 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Aug 7, 2015 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Aug 7, 2015 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Aug 7, 2015 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Aug 7, 2015 View changes

This comment has been minimized.

This comment has been minimized.

Hide details View details kevingranade merged commit ca58abb into CleverRaven:master Aug 7, 2015 1 check passed

1 check passed

This comment has been minimized.

chaosvolt commented Aug 7, 2015

This comment has been minimized.

VlasovVitaly commented Aug 7, 2015

This comment has been minimized.

chaosvolt commented Aug 7, 2015

This comment has been minimized.

VlasovVitaly commented Aug 7, 2015

This comment has been minimized.

chaosvolt commented Aug 7, 2015

This comment has been minimized.

CIB commented Aug 8, 2015

VlasovVitaly referenced this pull request Aug 8, 2015

VlasovVitaly deleted the VlasovVitaly:fix-filenames-tiles-loading branch Aug 11, 2015

drbig referenced this pull request Aug 12, 2015

VlasovVitaly commented on `src/path_info.cpp` in `e7ff724` Aug 1, 2015

DavidKeaton reviewed Aug 1, 2015

View changes

BevapDin reviewed Aug 7, 2015

View changes

BevapDin reviewed Aug 7, 2015

View changes

BevapDin reviewed Aug 7, 2015

View changes

BevapDin reviewed Aug 7, 2015

View changes

BevapDin reviewed Aug 7, 2015

View changes

kevingranade merged commit `ca58abb` into CleverRaven:master Aug 7, 2015
1 check passed