[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #36, patch #4830 (project freeciv): Firstly, thanks for prepare my patch to state it was applied to Freeciv code(trunk). I also noticed there's first policy in experimental ruleset. Tnis policy is called Personal Freedom. I must notice that by setting Personal Freedom at the beginning of game, I have 51 science output in republic with only two cities. This cities have size 4 (each). In each I have build market and library. I'm no inventing university, so probably I will have science output at 70. In my opinion that's too large output. I have idea about AI. AI can use predefined policies settings for state, like in war or in pace. ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #35, patch #4830 (project freeciv): Original forum posts about this feature and possible uses: one http://forum.freeciv.org/f/viewtopic.php?f=13t=311, two http://forum.freeciv.org/f/viewtopic.php?f=13t=310, three http://forum.freeciv.org/f/viewtopic.php?f=13t=419. ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Update of patch #4830 (project freeciv): Status: Ready For Test = Done Open/Closed:Open = Closed ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Update of patch #4830 (project freeciv): Status: In Progress = Ready For Test ___ Follow-up Comment #28: - Send player's real multipliers info only when INFO_FULL sent - Removed multipliers from experimental ruleset (there was no effects using them, anyway) (file #21583) ___ Additional Item Attachment: File name: Multipliers-3.patch.bz2Size:10 KB ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #29, patch #4830 (project freeciv): In multipliers.c: do multipliers, multiplier_count, and multiplier_used need the static keyword? I had thought this was necessary for persistent variables defined at file scope. In handle_ruleset_multiplier(), is there a reason to use get_multiplier_count()+1 rather than get_multiplier_count()++? For the AI: I presume the plan is to have the AI use the default values for this ticket, and add AI adjustments in a later ticket? ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #26, patch #4830 (project freeciv): I'm making a new version of the patch (so I hope you don't update it just now, to avoid duplicate work, and possibly work that cannot be merged). Some notes already, for future submissions: - Make sure that the patch compiles against the current version at the time of submitting it. Maybe we were just unlucky and this one got bitrotten very fast (since 19-Jul you submitted latest version), but there was several places needing updating - Change your editor settings so that no actual tabulators are added, but only spaces - Probably comes from editor settings too: Make sure no extra spaces are added to the end of the line. Empty lines should really be empty and not filled with spaces to the same indentation than surrounding lines ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #27, patch #4830 (project freeciv): - Updated against svn - Dropped diff_ignore changes - Ruledit to save multipliers to governments.ruleset and not to separate file. Also saving of the helptext was missing - In ruleset.c, merged multipliers loading to governments.ruleset handling functions - handle entire ruleset at once Also in case of error just set ok = FALSE instead of returning directly so that if cleanup handling changes in the future, not every single failure condition needs changing - Replaced tabulators with spaces - Removed extra spaces after the lines - Renamed send_ruleset_multiplier() as send_ruleset_multipliers() as it sends all of them - Typofixes in log messages - Changed one ruleset loading error logging from log_error() to ruleset_error() - Renamed multiplier_free() as multipliers_init() (plural + it really is init) - Added empty multipliers_free() (plural) - Cleaned up calls to multipliers_init() and multipliers_free(). They happen in game_ruleset_ini() / game_ruleset_free() for all the programs, and only there. Client and server (but not utility programs) used to call _init() directly, but _free() through game_ruleset_free(). - Renamed parameter of multiplier_index() from pach (copypaste from achievement code?) to pmul - Removed client/include/multipliersdlg.h (should have been named multipliersdlg_g.h if remained) since client common code does not call these functions - Added gui-gtk2/gui-gtk3 gamedlgs.h with prototype of popup_multiplier_dialog(), removed local duplicate prototype from gamedlgs.c - ++multiplier - multiplier++ in create_multiplier_dialog() - Removed empty line added to boot_help_texts(), with no other nearby edits - Added comment /* FC__MULTIPLIERS_H */ to #endif ending multipliers.h - Added rule_name to ruleset_multiplier packet, dropped translated_name as client translates it itself. send_ruleset_multipliers() was sending rule_name (as actual name) and untranslated_name (as translated name) - Removed extra spaces added to empty line in player.c:player_new(), savegame2.c:sg_save_player_main() etc - Experimental ruleset multipliers help to follow the usual ruleset documentation format - Added ruleset documentation comments to all rulesets - Moved zeroing of 'i' to initialization of for() loops in handle_player_multiplier() - ++i - i++ in handle_player_multiplier() - Moved pmul variable inside the only block where it's needed in handle_player_multiplier() - Send player info once server has handled adjustments to multipliers. For the client of player him/herself this should confirm the change, for other players' it's new information - Changed savegame multiplier entry multipler - multiplier Not yet an commit candidate: - package_player_info() always sets real values. Should require at least embassy info level to see other players' policy settings - The examples in experimental ruleset should go to separate patch if at all (requires also documentation in doc/README.ruleset_experimental) (file #21528) ___ Additional Item Attachment: File name: Multipliers-2.patch.bz2Size:10 KB ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Update of patch #4830 (project freeciv): Assigned to:None = cazfi ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Update of patch #4830 (project freeciv): Category:rulesets = general ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #25, patch #4830 (project freeciv): I already assigned the patch to myself, and will get to it as soon as I can. ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #21, patch #4830 (project freeciv): - The idea of multipliers should extends to different part of Freeciv, so it's not related only to governments - Changes to diff.ignore are removed - Weird changes to packets.def are removed - ai/default/daieffects.c does multiply value of effect by value of multiplier - Someone told me that number of packets can be fixed on merge time - C++ comments are probably removed - I have fixed brace locations of if and for - This division and multiply are necessary to set multiplier to step multiplier] - File name in Makefile.am are fixed - Added header probably to all function that needed header - Added /* common */ section identifier to multipliers.h - Indentation are fixed in some places - Added documentary string to multipliers.ruleset - Fixed ++x to x++ in mutliplier_new - In game.c exists call to multiplier_free, because ruleset going to be freed - Added blank line to some places between variable initialization and code - Changed untranslated_name to rule_name - Player's multipliers are initialized (set's to default value) in player creation code - Added code to save ruleset (file #21405) ___ Additional Item Attachment: File name: multipliers.patch Size:47 KB ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #22, patch #4830 (project freeciv): The idea of multipliers should extends to different part of Freeciv, so it's not related only to governments What the controls in gui then represent if not (governing) policies? If you dislike adding them to governments.ruleset, maybe game.ruleset? It already has things like disasters and achievements, of which there can be 0 to several (but not to hundreds) in a ruleset (similarly terrain.ruleset packs many object types) Patch is already getting quite good, but read it (the patch file itself) through once more to catch unintentional changes - there's still changes like replacing empty line with a line with some spaces etc. Also, at least in two files there was no empty line between last #include from previous section and the comment telling what directory the next one is. Someone told me that number of packets can be fixed on merge time Yeah, if there comes changes between submitting the final version and commit time, those can be added. But patch really should apply and compile against svn HEAD at the time it's submitted. Can you add gui-gtk2 version at least? Changes between gtk2 gtk3 clients are usually relatively easy to port. ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #19, patch #4830 (project freeciv): 1. SDL client compiles 2. - New rules should be saved to ruleset in tools/ruledit/rulesave.c Saved to this file should be ruleset? ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #20, patch #4830 (project freeciv):
- I think these are mainly going to be related to governments, and you even
name the dialog currently as government modifiers. It would probably be
better to add them to governments.ruleset than to add completely new ruleset
file, especially one that often only contains documentary comments.
- Unrelated changes to diff.ignore that didn't even apply cleanly. If you're
actively using diff.ignore, please submit that as separate patch
- There's some other noise changes (and quite weird ones at the: changing
one of the lines of the C-comment /* */ to begin was C++ comment //, replacing
packets. with (* packets. *) in packets.def description.) These were just
the ones I noticed while rebasing parts not applying cleanly.
- Effect values are also checked in ai/default/daieffecs.c, at least. That
needs similar update to effects.c change
- Renumbered packets as there's new 241 introduced in svn
- There's more C++ -comments (//) to remove
- Fix brace locations:
if () {
...
} else {
...
}
- Is / m-step * m-step in multipliers_command_callback() missing braces as
it does nothing except losing some accuracy ue to int arithmetics (first
divide by m-step and then multiply back)
- To client/include/Makefile.am header list multipliers.h is added but
actually the file is called multipliersdlg.h
- All new functions need function header
- multiplier.h is lacking /* common */ section for headers included from
common/
- *A lot* of weird indentation
- Rulesets need to be documented (commented)
- When it doesn't have functional difference, use x++ instead of ++x -
multiplier_new()
- multiplier_free() is called from both common/game.c and client_main.c
srv_main.c. Former is the correct one, I think (currently function
implementation is such that double-call does no harm, but better to get it
right)
- Variable declarations belong to the beginning of the block, followed by
empty line before actual code
- load_ruleset_effects() should use multiplier rule_name(), not
untranslated_name()
- Loading old savegame formats in savegame.c also need to set multipliers to
default values (or does this come due some initialization?)
- New rules should be saved to ruleset in tools/ruledit/rulesave.c
Rulesave.c is part of ruleset editor tool. You need to make saving part
equivalent to loading part in server/ruleset.c for your ruleset format
changes.
___
Reply to this item at:
http://gna.org/patch/?4830
___
Message sent via/by Gna!
http://gna.org/
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #17, patch #4830 (project freeciv): Added savegame/loadgame functionality. Still there's no AI code, but I'm not AI expert. I think the patch is ready for inclusion. (file #21272) ___ Additional Item Attachment: File name: multipliers.patch Size:39 KB ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #18, patch #4830 (project freeciv): Still just scrolled through the code (I hope to test it in practice and have more detailed look within a week) - Do other clients even compile? You add function to client interface, but the clients seem not to have even empty implementation to provide the symbol. Real implementation for gtk2-client should be easy to add anyway. - New rules should be saved to ruleset in tools/ruledit/rulesave.c - Header comment (can be mostly copied from some other ruleset) to new ruleset files needed, format of the multiplier sections need to be documented in all rulesets - No new functions without function header are accepted - Some uncommented code lines remain (read the patch file through yourself to notice such cases of changes not you have not meant to include) ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #15, patch #4830 (project freeciv): Attached a version of lachu's file #21193 rebased to recent trunk (over patch #4851, patch #4822, patch #4826) so I could play with it. No other changes. (file #21235) ___ Additional Item Attachment: File name: trunk-multipliers-file21193-rebased.patch Size:35 KB ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #16, patch #4830 (project freeciv): A couple of comments on the substance (get_target_bonus_effects()): * I think it would be useful to be able to have each step make less than 1.0 difference to a particular effect. A simple way to do this would be to scale by 100; so if I want a multiplier-based effect where every step of 1 has an effect of 1, I specify value=100. * In the !target_player case, I think effects with multipliers should have zero effect, not peffect-value. (I have lots of half-baked ideas about where to take this feature after the basic patch is ready, which I'll create a new ticket for rather than polluting this one.) ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #13, patch #4830 (project freeciv): I think this is not yet ready for inclusion. There's missing save game and load game facilities. There's also AI code missing. ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #14, patch #4830 (project freeciv): I have fixed all typos and changed what you tell is to change. (file #21193) ___ Additional Item Attachment: File name: multipliers.patch Size:36 KB ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #11, patch #4830 (project freeciv): Patch version, which adds multipliers to help dialog. Setting two variables in ruleset_free(game.c) to zero is needed for cleanup purposes. If we don't do that, then in special cases (for example ruleset was loaded many times) we can't load all multipliers. (file #21186) ___ Additional Item Attachment: File name: multipliers.patch Size:32 KB ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #12, patch #4830 (project freeciv): On the global variables: I agree they are needed for cleanup purposes, but I still think they should be scoped to multipliers.c. Consider adding helper functions to game_ruleset_init() and game_ruleset_free() that operate on static variables in multipliers.c: functions in multipliers.c will be able to see these values, but they will not be directly accessible from other code. For example, the achievements code stores a static array locally scoped to achievements.c, which is initialised and cleared with functions called from game.c. Doing it this way means they are automatically processed however the ruleset is loaded, so you don't need to maintain separate initialisations in client_main and srv_main (and are no longer missing one in ruledit, civmanual, or any other ruleset-consuming tools that get added in the future). r25288 bumped the packet ID count, so your packets need renumbering, but that's very minor (and can be fixed on commit, rather than being an endless chase). Similarly, the network capstring in fc_version will need to be updated with the application of this patch (this, again, can be added on commit). I still worry about the non-static stuff in GTK, but don't know enough to be certain. In helpdlg_g.h, you have Policiess, when I think you want Policies. In handle_ruleset_multiplier(), you have Too many multipliers sended by server, when I think you want Too many multipliers sent by the server. In experimental/multipliers.ruleset, you have Does nothink, when I think you want Does nothing. In helpdata.txt, you have ...which defined your civilization rights, when I think you want ...which define your civilization's rights.. Also in helpdata.txt, consider using a _() macro so that the text string is translated. As may be indicated by the increasingly minor and detailed nature of my feedback, I think this is almost ready for inclusion. ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #9, patch #4830 (project freeciv): Thanks for the updates: this is lots cleaner and easier to work with. Applies smoothly over r25244, and builds. Naming: Most of the rest of the codebase uses singular nouns for most purposes, with plural nouns only used when specifically handling multiple things (and not as a list). To follow this, most uses of multipliers should probably be multiplier. handle_player_mulitpliers() This probably belongs in server/plrhand.c, in a similar manner to handle_player_rates(), handle_player_change_government, etc. CodingStyle mandates declaring variables at the beginning of the scope, followed by a blank line. multipliers.ruleset A null ruleset (not exposing any multipliers) needs to be supplied for all the other rulesets (excluding experimental). Currently, the server crashes on start (can't load ruleset) with the default ruleset. This crash persists even if one starts the server with -r experimental.serv load_ruleset_multipliers() I'd put this between load_game_names() and load_technology_names() just because I like alphabetical order, but it's not important. send_ruleset_multipliers() The dl_send_packet_ruleset_multiplier() call runs over 77 characters multiplier_count I don't see this initialised anywhere on the client: packhand.c seems to assume it to be 0 without checking. I also don't understand why this is set to 0 in game_ruleset_free(): what is the intent here? multiplier_used I don't see this initialised anywhere on client or server. Aside from the apparent useless set to 0 in game_ruleset_free(), it also does not appear to be used outside multipliers.c, so if probably ought be scoped to only that file. game.c: With the removal of the rest of these variables, this file no longer needs to include multipliers.h effects.h: I still like pointers to data structures, rather than indexed access to arrays :) get_target_bonus_effects() This code would benefit from modification of the comment to describe the new behaviour. popup_multipliers_dialog() I don't know much about GTK, but I would have expected this to be a static function, or to be declared in a header file somewhere. ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Follow-up Comment #10, patch #4830 (project freeciv): Oh, and I forgot, there probably ought be some way the ruleset author can supply helptext for each multiplier, and a facility in client/helpdata.c to parse that and display it in the online help. ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #4830] Multipliers to effects
Update of patch #4830 (project freeciv): Summary: Multiplers to effects = Multipliers to effects ___ Reply to this item at: http://gna.org/patch/?4830 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
