[Freeciv-Dev] [bug #22194] Update move_points_text() for move_fragments
Update of bug #22194 (project freeciv): Status: Ready For Test = Fixed Open/Closed:Open = Closed ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #12, bug #22194 (project freeciv): So, what should I do with this? Reduce movepoints to lowest terms: * always? * only in selected situations in the help, e.g. * Ignores terrain effects (moving costs at most 1/3 MP per tile) even with 9 move_fragments? * never? * something else? ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #13, bug #22194 (project freeciv): only in selected situations That's the only way to get it always right, I think. Isn't it quite simple by adding boolean parameter for move_points_text()? The only thing where I've noticed current code to be broken is the active unit display I mentioned, where the numbers change in a hard-to-follow way when unit is being moved by keyboard (goto of course is not so much affected) ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Update of bug #22194 (project freeciv): Status: Fixed = Ready For Test ___ Follow-up Comment #14: only in selected situations That's the only way to get it always right, I think. Isn't it quite simple by adding boolean parameter for move_points_text()? Attached patch does this, and code now only reduces when talking about unit types or other ruleset properties in the abstract. And now that I read the patch, you seem to have forgotten part of CodingStyle that says that there's empty line between declaration of variables and actual code. Corrected most of these. However, for the GCD-calculating block, inserting a blank line to a three-line block seems unnecessarily ugly. I think strict application of the rule to non-function-body blocks like this discourages declaring variables at their minimum possible scope. persia wrote (comment #5): My concern was with rulesets that set SINGLE_MOVE to 0, You're right, this should be an error at ruleset load time (probably also for igter_cost). Now patch #4834. (file #21110, file #2) ___ Additional Item Attachment: File name: trunk-movefrags-noreduce.patch Size:24 KB File name: S2_5-movefrags-noreduce.patch Size:17 KB ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Update of bug #22194 (project freeciv): Status: Ready For Test = Fixed ___ Follow-up Comment #9: it's worth reducing fractional MP to their lowest terms I've been thinking that, but it's probably better to the constant divider [...] Oops, I left this on my to-commit branch without this question being fully resolved, so now it's committed. I can revert or modify the lowest-terms behaviour if you feel strongly. Leaving ticket open until this is resolved. ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #10, bug #22194 (project freeciv): Leaving ticket open until this is resolved. I think this patch had also internal conflict of goals. You calculate denomlen for the constant denominator, but then you display MP reduced to lowest terms. So if move_fragments = 10, you get denomlen == strlen(10) == 2, but in case of 2/10 display just 1/5. And now that I read the patch, you seem to have forgotten part of CodingStyle that says that there's empty line between declaration of variables and actual code. ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #11, bug #22194 (project freeciv): I think this patch had also internal conflict of goals. You calculate denomlen for the constant denominator, but then you display MP reduced to lowest terms. So if move_fragments = 10, you get denomlen == strlen(10) == 2, but in case of 2/10 display just 1/5. That's deliberate; when 'align' is requested, it's because there's going to be a whole fixed-width table of these, so alignment has to assume the worst case. (This is used only in the tables of veteran properties.) Veteran level Power factor Move bonus green 100% - veteran150% + 1/20 hardened 175% + 1/ 5 elite 200%+ 2 11/20 you seem to have forgotten part of CodingStyle that says that there's empty line between declaration of variables and actual code. Oops, I hadn't internalised that (despite going over all of CodingStyle recently). Will see if an opportunity arises to fix. ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
URL: http://gna.org/bugs/?22194 Summary: Update move_points_text() for move_fragments Project: Freeciv Submitted by: jtn Submitted on: Sat 14 Jun 2014 21:28:31 BST Category: None Severity: 3 - Normal Priority: 5 - Normal Status: In Progress Assigned to: jtn Originator Email: Open/Closed: Open Release: Discussion Lock: Any Operating System: Any Planned Release: 2.5.0, 2.6.0 ___ Details: move_points_text(), which renders possibly-fractional movements points to text, needs fixes/improvement: * It calculates padding once and stores it in a static variable, which isn't appropriate now that SINGLE_MOVE is mutable (indeed, probably it always gets it wrong if called while SINGLE_MOVE==0) * Now that SINGLE_MOVE can be non-prime, it's worth reducing fractional MP to their lowest terms (2/6 = 1/3). ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #1, bug #22194 (project freeciv): Now that SINGLE_MOVE can be non-prime, it's worth reducing fractional MP to their lowest terms (2/6 = 1/3). I've been thinking that, but it's probably better to the constant divider, so that when one moves 1/6 cost road, unit moves left go: 1 - 5/6 - 4/6 - 3/6 - 2/6 - 1/6 - 0, and not 1 - 5/6 - 2/3 - 1/2 - 1/3 - 1/6 - 0 ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #2, bug #22194 (project freeciv): SINGLE_MOVE shouldn't be 0 (this leads to issues like infinite attack count). Maybe this ought be checked in ruleset sanity? ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #3, bug #22194 (project freeciv): SINGLE_MOVE shouldn't be 0 Apparently this can happen in the client prior to ruleset loading (according to an existing comment). ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #4, bug #22194 (project freeciv): That's fine: before ruleset load, we shouldn't need to calculate either the remaining movement left to a unit after performing activities, or a textual representation of the movement rate. My concern was with rulesets that set SINGLE_MOVE to 0, or having it set to that value during gameplay. ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Follow-up Comment #5, bug #22194 (project freeciv): My concern was with rulesets that set SINGLE_MOVE to 0, You're right, this should be an error at ruleset load time (probably also for igter_cost). or having it set to that value during gameplay. move_fragments can't change during gameplay, fortunately. ___ Reply to this item at: http://gna.org/bugs/?22194 ___ 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] [bug #22194] Update move_points_text() for move_fragments
Update of bug #22194 (project freeciv): Status: In Progress = Ready For Test ___ Follow-up Comment #6: it's worth reducing fractional MP to their lowest terms I've been thinking that, but it's probably better to the constant divider, so that when one moves 1/6 cost road, unit moves left go: [...] Well, hm. On the one hand, not dividing it down shows the 'bones' of the ruleset in help like Moving along River costs 3/9 MP. If most of the ruleset is based on 1/3 MP and 1/9 is reserved for some special case, forcing everything to n/9 looks ugly. On the other hand, expecting people to do fractional arithmetic to interpret how many moves they have left might be a bit much. Really what you wanted when a unit's moving along a road is appropriate steps for that road; [G]oto will tell you how many tiles you can move. Well, attached patch reduces unconditionally so you can see what it looks like. Perhaps I could another flag to move_points_text() controlling whether reduction is done, so that unit MP is reported without reduction but other contexts have it? (file #21015, file #21016) ___ Additional Item Attachment: File name: trunk-move-fragments-display.patch Size:6 KB File name: S2_5-move-fragments-display.patch Size:7 KB ___ Reply to this item at: http://gna.org/bugs/?22194 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev