News:

Want to praise Simutrans?
Your feedback is important for us ;D.

Renaming and translation of the code

Started by Markohs, January 29, 2013, 11:14:35 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Markohs

Do you mind I rename it now and upload to trunk, kieron?

kierongreen

By all means. I won't have proper internet access for some time but will update this patch when I can to take into account changes to trunk.

Markohs

#2
Done, uploaded @6299

EDIT: Dwachs, changed api_world.cc:


    STATIC register_method< bool(karte_t::*)(koord) const>(vm, &karte_t::ist_in_kartengrenzen,  "is_coord_valid");


    STATIC register_method< bool(karte_t::*)(koord) const>(vm, &karte_t::is_in_map_limits,  "is_coord_valid");


Can this break scenarios?

Ters

is_within_map_limits sounds better to me, or maybe just is_within_map. Or even is_coord_valid. What's the difference between the grid and the map anyway?

Markohs

#4
Yes, I was precisely thinking the same thing in the bus one hour ago. is_in_map_limits sounds so much better than ist_in_kartengrenzen that didn't noticed it until later. the name I chosed was a loo literal translation Iguess, it's what google translator suggested. :)   In fact I was quite sure in the bus too a Norwegian guy whould quote me and point me precisely this in the forum. ;)

I'll change to is_within_map_limits and is_within_grid_limits. before someone starts using the new names.

Given a x,y tiles long map, the map_limits is a x*y sized array, coords go from ( 0 .. x-1 ,0 .. y-1 ) each position represents a TILE, has a planquadrat_t iirc.

grid_limits sizes grid_hgts, and each point represents the height of a CORNER (shared by a maximum of 4 tiles), a vertex, so it has one more position in each dimension.

(0..x,0..y)

For example, a 8x8 map contains 8x8 TILES, and is represented by a 9x9 grid of heights.

Dwachs

Quote from: Markohs on January 29, 2013, 12:43:56 PM
Can this break scenarios?
No. As long as you do not change the function signature, such a change has no impact on the scenario script side.
Parsley, sage, rosemary, and maggikraut.

prissi

If you translate is, I propose rather is_valid_koord() (or even is valid pos, as the posiition is the be queried) and is_valid_pos_grid(). The latter, because the grid is not used so often, and the connection is more clear this way, imho.

Markohs

#7
is_valid_pos() and is_valid_pos_grid() then. koord is german, no?

Maybe now that we are changing this, we can change this extra ones:


get_jahreszeit -> get_season
get_groesse_x -> get_dimension_x (or size?)
get_groesse_y ->  get_dimension_y (or size?)
get_groesse_max ->  get_dimension_max (or size?)
cached_groesse_gitter_x -> cached_dimension_x -> same for y and all related variables/functions.
get_schlaf_zeit -> get_idle_time (or get_sleep_time?)



ansicht_ij_off -> view_ij_off
set_ansicht_ij_offset -> set_view_ij_offset
get_ansicht_ij_offset -> get_view__ij_offset

groundwasser and related: -> waterlevel ? ocean_level?

markiere -> mark
unmarkiere -> unmark (and related)

Dwachs

I like the new translation is_within_X_limits.

While you  are at translating things, some of these dimension may be put into koord-structs to get rid of these groesse_x stuff.

You can go ahead and change these things, but then please translate/create proper doxygen-style comments.

I propose to translate get_groesse_x to get_size_x, and anything gitter/grid related translate to *_grid_size_*
Parsley, sage, rosemary, and maggikraut.

Markohs

mmm... I'll get my hands into this then. Don't worry all will be documented.

I also liked within_limits because a coord outside limits it's not invalid per-se, depending of the meaning of the coord. If we allowed to use the inspect tool outside the map, whould be valid and outside limits.

With the water_level project with the raise/lower tools I have to use fictional tile coords, for example a tile (64,64) position, in a 64x64 map, that's right now not a valid tile coordinate, to be able to raise the SE corner of the map (right now that tool only raises NW corners of "valid" tiles ). You can also consider that coordinate is a grid coord, or a outside limits tile coordinate, but that doesn't make it invalid. Depends on the point of view.

prissi

#10
is_within_ is also ok. Just is_in_map_limit() is an ugly translation. Get_groesse_x stems from pre-koord era. Thus returning a koord there is fine with me. It is also no used in any critical prce, I think.

The get_groesse_x() and get_groesse_y() are best replaced by a single get_size() which returns a koord.

But again, I am not to keen to translate too much. I would not translate local variables. This could just introduce new errors for exactly no much gain. Same with mark and unmark, I think one routine did at least used this together with markiere. (May be wrong, before renaming check it.)

get_grundwasser -> get_waterlevel() (which will require a lot of search an replace in Kierons patch I think)

T0m4S

#11
I think with your changes there's a problem. With revision 6303 I can't compile on windows 7 with MSVC 2008. I get the following errors:

1>.\simwerkz.cc(904) : error C2039: 'is_within_map_limits' : is not a member of 'karte_t'
1>        c:\simutrans_svn\simutrans\trunk\simworld.h(90) : see declaration of 'karte_t'
1>.\simwerkz.cc(970) : error C2039: 'is_within_map_limits' : is not a member of 'karte_t'
1>        c:\simutrans_svn\simutrans\trunk\simworld.h(90) : see declaration of 'karte_t'
1>.\simwerkz.cc(1056) : error C2039: 'is_within_map_limits' : is not a member of 'karte_t'
1>        c:\simutrans_svn\simutrans\trunk\simworld.h(90) : see declaration of 'karte_t'
1>.\simwerkz.cc(1056) : error C2039: 'is_within_map_limits' : is not a member of 'karte_t'
1>        c:\simutrans_svn\simutrans\trunk\simworld.h(90) : see declaration of 'karte_t'
1>.\simwerkz.cc(1306) : error C2039: 'is_within_map_limits' : is not a member of 'karte_t'
1>        c:\simutrans_svn\simutrans\trunk\simworld.h(90) : see declaration of 'karte_t'
1>.\simwerkz.cc(3146) : error C2039: 'is_within_map_limits' : is not a member of 'karte_t'
1>        c:\simutrans_svn\simutrans\trunk\simworld.h(90) : see declaration of 'karte_t'
1>.\simwerkz.cc(4199) : error C2039: 'is_within_map_limits' : is not a member of 'karte_t'
1>        c:\simutrans_svn\simutrans\trunk\simworld.h(90) : see declaration of 'karte_t'
1>.\simwerkz.cc(4795) : error C2039: 'is_within_map_limits' : is not a member of 'karte_t'
1>        c:\simutrans_svn\simutrans\trunk\simworld.h(90) : see declaration of 'karte_t'
1>simwin.cc
1>simworld.cc
1>.\simworld.cc(1648) : warning C4244: 'argument' : conversion from 'const sint16' to 'sint8', possible loss of data
1>.\simworld.cc(1946) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(2020) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(2104) : error C3861: 'is_within_grid_limits': identifier not found
1>.\simworld.cc(2131) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(2148) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(2220) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(2312) : error C3861: 'is_within_grid_limits': identifier not found
1>.\simworld.cc(2339) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(2451) : warning C4244: 'argument' : conversion from 'uint16' to 'uint8', possible loss of data
1>.\simworld.cc(2844) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(2984) : warning C4244: 'argument' : conversion from 'double' to 'const uint32', possible loss of data
1>.\simworld.cc(3706) : warning C4267: 'argument' : conversion from 'size_t' to 'uint', possible loss of data
1>.\simworld.cc(4189) : error C3861: 'is_within_grid_limits': identifier not found
1>.\simworld.cc(4690) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(5147) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(5355) : error C3861: 'is_within_map_limits': identifier not found
1>.\simworld.cc(5911) : error C3861: 'is_within_map_limits': identifier not found
1>skin_reader.cc


I'll try on linux asap.

Edit: same problem on linux:

simwerkz.cc: In member function 'virtual const char* wkz_raise_t::work(karte_t*, spieler_t*, koord3d)':
simwerkz.cc:904:11: erreur: 'class karte_t' has no member named 'is_within_map_limits'
simwerkz.cc: In member function 'virtual const char* wkz_lower_t::work(karte_t*, spieler_t*, koord3d)':
simwerkz.cc:970:11: erreur: 'class karte_t' has no member named 'is_within_map_limits'
simwerkz.cc: In static member function 'static const char* wkz_setslope_t::wkz_set_slope_work(karte_t*, spieler_t*, koord3d, int)':
simwerkz.cc:1056:15: erreur: 'class karte_t' has no member named 'is_within_map_limits'
simwerkz.cc:1056:73: erreur: 'class karte_t' has no member named 'is_within_map_limits'
simwerkz.cc: In member function 'virtual const char* wkz_marker_t::work(karte_t*, spieler_t*, koord3d)':
simwerkz.cc:1306:11: erreur: 'class karte_t' has no member named 'is_within_map_limits'
simwerkz.cc: In member function 'const char* wkz_station_t::wkz_station_dock_aux(karte_t*, spieler_t*, koord3d, const haus_besch_t*)':
simwerkz.cc:3146:14: erreur: 'class karte_t' has no member named 'is_within_map_limits'
simwerkz.cc: In member function 'const char* wkz_depot_t::wkz_depot_aux(karte_t*, spieler_t*, koord3d, const haus_besch_t*, waytype_t, sint64)':
simwerkz.cc:4199:11: erreur: 'class karte_t' has no member named 'is_within_map_limits'
simwerkz.cc: In member function 'virtual const char* wkz_headquarter_t::work(karte_t*, spieler_t*, koord3d)':
simwerkz.cc:4795:11: erreur: 'class karte_t' has no member named 'is_within_map_limits'
make: *** [build/default/simwerkz.o] Erreur 1

Markohs

Are you sure you updated simworld.h too?

T0m4S

I did svn update. So it should be updated, shouldn't it?

Markohs

 Do you have local modifications to your files? that can maybe have triggered conflicts, try moving simwerkz.cc.* to another directory and re-update please.

My guess is that you already modified that file and it's in conflict. if the file is missing svn will restore it.

T0m4S

Actually I think that you forgot to change the file simwerk from rev 6301 to 6303. In revision 6303 this file is not modified while the others are.

Markohs

You are right, fixing it. Just give me a few minutes so I do all the translations in a single commit.

Markohs

Corrected in last update, please confirm me you can compile now.

T0m4S

QuoteCorrected in last update, please confirm me you can compile now.
Yes, it compiles now! :)

Markohs

#19
Posting the changes from svn I plan to add, please have it a look si I don't have to do multiple commits like has happened and I don't make so much noise. :) It's WIP, not finished yet.

https://dl.dropbox.com/u/30024783/translation.patch


TODO:

karte_t::get_size_grid() -> has to be renamed to get_grid_size, it sounds weird like this. get_size() is too generic maybe, and there are classes that have that method name already, better to choose different names I think.

settings_t:: get_groesse_x -> settings_t:: get_size_x
ansicht_ij_off -> view_ij_off
get_jahreszeit -> get_season

T0m4S

Quote from: Markohs on January 30, 2013, 06:02:06 PM
Posting the changes from svn I plan to add, please have it a look si I don't have to do multiple commits like has happened and I don't make so much noise. :) It's WIP, not finished yet.

https://dl.dropbox.com/u/30024783/translation.patch


TODO:

karte_t::get_size_grid() -> has to be renamed to get_grid_size, it sounds weird like this. get_size() is too generic maybe, and there are classes that have that method name already, better to choose different names I think.

settings_t:: get_groesse_x -> settings_t:: get_size_x
ansicht_ij_off -> view_ij_off
get_jahreszeit -> get_season
It compiles without problems.

Dwachs

karte_t::get_size sounds reasonable. I would call the other method get_grid_size.
Parsley, sage, rosemary, and maggikraut.

Markohs

blick_aendern -> what do you prefer, change_view or move_view ?

Markohs

BTW I noticed scenarios code is failing now, to check if it was my changes I downloaded nightly 6298 and 6296, they still fail.

Downloading more versions the bug shows somewere between

sim-wingdi_2013-01-17_v112.1_r6284.zip <- Fails
sim-wingdi_2013-01-16_v112.1_r6279.zip <- Works


Markohs

Almost finished, comments welcome, there are still a few things to translate and document, but not much, I tried to not overdo the translating so it doesn't affect much extra files.

https://dl.dropbox.com/u/30024783/translation_v2.patch

Dwachs

Quote from: Markohs on January 31, 2013, 11:23:49 AM
BTW I noticed scenarios code is failing now, to check if it was my changes I downloaded nightly 6298 and 6296, they still fail.
sim-wingdi_2013-01-17_v112.1_r6284.zip <- Fails
sim-wingdi_2013-01-16_v112.1_r6279.zip <- Works
None of the changes between 6279 and 6284 should have broken this part of the code. Is the file script/scenario_base.nut broken/missing/not up-to-date?
Parsley, sage, rosemary, and maggikraut.

Markohs

That was the problem, sorry. No bug then. :)

prissi

get_size should replace get_groesse_*(). But why the first lines of your patch repalce get_groesse by get_grid_size? That is certainly not what is intended. Because when you toate the map, stuff will be one position off.

Same for gameinfo, baum ... at many positions get_groesse is replaced by get_grid_size instead of get_size. Somehow this does not look like a clean translation patch when I see this.

Markohs

#28
I created 2 functions to replace get_groesse_x and get_groesse_y:

get_size
get_grid_size

On a 64x64 map, the first one will return (63,63), and the second one will return (64x64) This comes from the fact  that some parts of the code care for the grid, that's a 65x65 (from 0 to 64) and other iterate over planquadrat_t items, that are stored on a 64x64 (from 0 to 63) array. Those 2 kind of algorithms just used get_groesse_? functions, and many of them looked like:

for (i=0 ; i < welt->get_groesse_x()-1 ; i++){
// iterate over planquadrats
}

This functions had to do that minus 1 operation, that was (I know, performance difference will be near zero) sightly slower. And I assume they just executed once because the function is const, but if any operation inside the loop was not const, it whould have triggered the operation each iteration.

Now they are:

for (i=0 ; i < welt->get_size().x ; i++){
// iterate over planquadrats
}

get_groesse_? functions already returned grid size, not map size.

Now you can see easily if the loops are iterating over the grid or the map easily, just looking wich of those 2 functions it's using. So it comes with a performance gain and a readibility improvement in my oppinion.

I could have created get_size alone and leave all those -1 around the code, but this feels more clean to me.

Anyway if you want me to revert that and just use get_size() I can do it. :)

In short:

get_groesse_x() == get_size().x +1 == get_grid_size().x

Markohs

 Well, ok, this is last version.

I've translated almost all function names. there is still some german in simworld.h where I coudn't find a translation clear enough, (remember I don't know german, I gust used google translator and tried to guess reading code).

I'm quite sure I didn't break anything, I just restrained myself to simworld.h, and just modified other files when there was no choice.

Pending of your comments, and from prissi to confirm me what to do on get_size(), won't commit yet.

This has been interesting, might go learning german some day myself. :)

prissi

Simutrans is in most parts not written for speed but for clarity, at least this is the intention. Thus if you operate on the map, you should use get_size() and if you operate on the grid get_grid_size().

I know you had good intentions, but this built in -1 will confuse more (as it already did to me) than a german function name. Thus get_size must return 64,64, as the size is 64 and get_grid_size() 65,65.

Also a for loop over the map will run from i=0 to i<64 if i[64]; The -1 is only for special situations for instance when an object is to be placed, and could run as well only to get_size()-get_size_of_object().

I admid that the -1 should have been documented in such cases. Actually, since a map can be rotated, such searches should run from 1 to get_size()-1 correctly.

Markohs

What you say makes sense, I have to agree with you. I'll change the patch like you say. :)

Markohs

Okay, ready, patch here.

replaced get_groesse_x() and get_groesse_y() with just get_size(), that returns a koord tuple, it's a const function so it should be as efficient as the previous implementation.

Tell me if you find it misses something. Comments and documentation welcome. I'll comeit this to svn if dwachs and prissi agree.

A list of what  future patches should adress:

*UNDOCUMENTED*

enum player_cost  : documented the values, not the enum, dunno what it's used for.
weighted_vector_tpl<gebaeude_t *> ausflugsziele;
slist_tpl<koord> labels;
marker_t marker;
step_mode;
message_t *msg;
sint32 average_speed[8];
uint32 tile_counter;
void world_xy_loop(xy_loop_func func, bool sync_x_steps);
static void *world_xy_loop_thread(void *);
message_t *get_message() { return msg; }
scenario_t *get_scenario() const { return scenario; }
void set_scenario(scenario_t *s);
convoihandle_t get_follow_convoi() const { return follow_convoi; }
settings_t const& get_settings() const { return settings; }
settings_t&       get_settings()       { return settings; }
bool is_fast_forward() const { return step_mode == FAST_FORWARD; }
void set_fast_forward(bool ff);
uint8 sp2num(spieler_t *sp);
spieler_t * get_spieler(uint8 n) const { return spieler[n&15]; }
spieler_t* get_active_player() const { return active_player; }
uint8 get_active_player_nr() const { return active_player_nr; }
void switch_active_player(uint8 nr, bool silent);
const char *new_spieler( uint8 nr, uint8 type );
void store_player_password_hash( uint8 player_nr, const pwd_hash_t& hash );
const pwd_hash_t& get_player_password_hash( uint8 player_nr ) const { return player_password_hash[player_nr]; }
void clear_player_password_hashes();
void rdwr_player_password_hashes(loadsave_t *file);
void remove_player(uint8 player_nr);
enum change_player_tool_cmds { new_player=1, toggle_freeplay=2, delete_player=3 };
void reset_timer();
void reset_interaction();
void step_year();
void set_ticks_per_world_month_shift(uint32 bits) {ticks_per_world_month_shift = bits; ticks_per_world_month = (1 << ticks_per_world_month_shift); }
sint32 get_time_multiplier() const { return time_multiplier; }
void change_time_multiplier( sint32 delta );
werkzeug_t *get_werkzeug(uint8 nr) const { return werkzeug[nr]; }
enlarge_map(settings_t const*, sint8 const* h_field);
karte_t();
~karte_t();
void add_convoi(convoihandle_t);
void rem_convoi(convoihandle_t);
vector_tpl<convoihandle_t> const& convoys() const { return convoi_array; }
void add_stadt(stadt_t *s);
void add_ausflugsziel(gebaeude_t *gb);
void remove_ausflugsziel(gebaeude_t *gb);
const weighted_vector_tpl<gebaeude_t*> &get_ausflugsziele() const {return ausflugsziele; }
void add_label(koord pos) { if (!labels.is_contained(pos)) labels.append(pos); }
void remove_label(koord pos) { labels.remove(pos); }
const slist_tpl<koord>& get_label_list() const { return labels; }
bool add_fab(fabrik_t *fab);
bool rem_fab(fabrik_t *fab);
int get_fab_index(fabrik_t* fab)  const { return fab_list.index_of(fab); }
fabrik_t* get_fab(unsigned index) const { return index < fab_list.get_count() ? fab_list.at(index) : NULL; }
const slist_tpl<fabrik_t*>& get_fab_list() const { return fab_list; }
bool cannot_save() const { return nosave; }
void set_nosave() { nosave = true; nosave_warning = true; }
void set_nosave_warning() { nosave_warning = true; }
bool sync_add(sync_steppable *obj);
bool sync_remove(sync_steppable *obj);
void sync_step(long delta_t, bool sync, bool display );    // advance also the timer
bool sync_eyecandy_add(sync_steppable *obj);
bool sync_eyecandy_remove(sync_steppable *obj);
void sync_eyecandy_step(long delta_t);    // all stuff, which does not need explicit order (factory smoke, buildings)
bool sync_way_eyecandy_add(sync_steppable *obj);
bool sync_way_eyecandy_remove(sync_steppable *obj);
void sync_way_eyecandy_step(long delta_t);    // currently one smoke from vehicles on ways
const checklist_t& get_checklist_at(const uint32 sync_step) const { return LCHKLST(sync_step); }
uint32 get_sync_steps() const { return sync_steps; }
void set_checklist_at(const uint32 sync_step, const checklist_t &chklst) { LCHKLST(sync_step) = chklst; }
const checklist_t& get_last_checklist() const { return LCHKLST(sync_steps); }
uint32 get_last_checklist_sync_step() const { return sync_steps; }
void command_queue_append(network_world_command_t*) const;
void clear_command_queue() const;
void network_disconnect();
void set_map_counter(uint32 new_map_counter);

*UNTRANSLATED FUNCTION NAME*

plans_laden_abschliessen; (what to translate to?)
set_dirty_zurueck;  (what to translate to?)
werkzeug  makes not much sense to translate without renaming werkzeug_t
grundwasser affects lots of places in the code.
zeiger whould require renaming zeiger_t
init_felder()  (what to translate to?)
bool can_ebne_planquadrat(koord pos, sint8 hgt, bool keep_water=false, bool make_underwater_hill=false) const;
bool ebne_planquadrat(spieler_t *sp, koord pos, sint8 hgt, bool keep_water=false, bool make_underwater_hill=false);

*UNTRANSLATED DOCUMENTATION*
long get_steps() const { return steps; }
uint32 get_schlaf_zeit() const { return idle_time; }
uint32 get_realFPS() const { return realFPS; }
uint32 get_simloops() const { return simloops; }
    void set_map_counter(uint32 new_map_counter);

*UNTRANSLATED NAME && DOCUMENTATION *
inline void markiere(const grund_t* gr) { marker.markiere(gr); }
inline void unmarkiere(const grund_t* gr) { marker.unmarkiere(gr); }
inline void unmarkiere_alle() { marker.unmarkiere_alle(); }

Dwachs

Looks good. Some comments:

-- spieler[]: maybe add a note: 1 = public player, 0 = standard human player
-- current season: 0..3 (0 .. spring ??)
-- msg: holds all the text messages in the messagebox (chat, new vehicle etc)
-- average speed (per way type) do determine speedbonus iirc
-- tile_counter: iirc this is used to distribute the workload when changing seasons to several steps
-- get_size: maybe better return 'koord const &' instead of 'koord' to avoid unnecessary copying ? - it seems not to be used in any time critical routine.

Otherwise: imho you can commit.
Parsley, sage, rosemary, and maggikraut.

prissi

It would be great next time, when you change anway thing contain "wrong" simutrans formatting "if (bla==1 && b) {" to change this also to "if(  bla == 1  &&  b  ) {" and so on. THis would get use closer to consistent formatted code too.