[Freeciv-Dev] [bug #22194] Update move_points_text() for move_fragments

2014-06-23 Thread Jacob Nevins
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

2014-06-22 Thread Jacob Nevins
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

2014-06-22 Thread Marko Lindqvist
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

2014-06-22 Thread Jacob Nevins
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

2014-06-16 Thread Jacob Nevins
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

2014-06-16 Thread Marko Lindqvist
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

2014-06-16 Thread Jacob Nevins
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

2014-06-14 Thread Jacob Nevins
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

2014-06-14 Thread Marko Lindqvist
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

2014-06-14 Thread Emmet Hikory
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

2014-06-14 Thread Jacob Nevins
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

2014-06-14 Thread Emmet Hikory
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

2014-06-14 Thread Jacob Nevins
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

2014-06-14 Thread Jacob Nevins
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