Log for on 13th April 2013:
Times are UTC Toggle Colours
00:33:15  *** frosch123 has quit IRC
07:20:47  *** fonsinchen has joined
07:20:47  *** ChanServ sets mode: +v fonsinchen
07:33:48  *** Supercheese has quit IRC
07:44:39  *** Alberth has joined
07:44:40  *** ChanServ sets mode: +v Alberth
09:06:52  *** fonsinchen has quit IRC
10:25:11  *** Zuu has joined
10:25:12  *** ChanServ sets mode: +v Zuu
10:29:29  *** fonsinchen has joined
10:29:29  *** ChanServ sets mode: +v fonsinchen
10:34:45  <Zuu> 50  -Fix(r25162): add missing parameter to parse_file_args:
10:34:45  <Zuu> 60  -Fix: same variable name was used twice when parsing recursive paths:
10:34:45  <Zuu> 70  -Fix: add license.txt with POXIS path separator also on windows, so that can find it:
10:34:45  <Zuu> 80  -Fix(r25145): forgot to add the .id and .title file for scenarios and hegihtmaps:
10:34:48  <Zuu> 90  -Fix(r25145): retain folders within script content:
10:35:44  <Zuu> 70 and 80 have been previously posted as musa-license-fix5.patch and musa-scen-hm-fix6.patch.
10:36:39  <Zuu> With 50 to 90 applied, hopefully uploading of heightmaps, scenarios and GSs should hopefully work.
10:36:51  <Alberth> you've been busy :)
10:36:59  <Zuu> AIs, AI libraries and GSs that uses sub folders are also fixed by 90.
10:37:28  <Zuu> s/GSs/GS libraries/
10:38:47  <Alberth> we usually just link to the directory with patch queues
10:39:08  <Zuu> Ok
10:39:42  <Zuu> But the patch queue file will not cary a description for the patch in hg? Or is that possible as I'v seen some git users do?
10:40:28  <Alberth> you can add a message to the patch
10:41:19  <Alberth>  -e to the qnew/qref command, it seems (and possibly with other commands too)
10:41:42  <Alberth> which gets added at the 2nd or 3rd line of the patch
10:41:56  <Zuu> I just saw, an example using qrefresh -m "one line message" or -e to create a multi-line message.
10:42:30  <Zuu> Good to know for future.
10:43:04  <Alberth> 60: files.add(path)  <-- that should not be renamed?  (I have no checkout to verify)
10:44:43  <Zuu> Alberth: Nope, files.add(path) add the path to the list of files that is returned from that method.
10:45:11  <Alberth> 70:    if root[:1] == '/':    interesting way to do  if root[0] == '/':    :)
10:45:11  <Zuu>
10:46:28  <Alberth> ah, thanks
10:46:34  <Zuu> Alberth: Isn't [:1] taking the last char of the string array?
10:47:31  <Alberth> nope, it's slicing   s[a:b]   means take the substring from index a upto, but excluding index b
10:48:13  <Alberth> where "[:"  means a == 0, and  ":]" means b == len(s)
10:48:27  <Alberth> negative index counts from the back, so you want root[-1]
10:48:42  <Zuu> Ok thanks
10:48:43  <Alberth> but first test that len(root) > 0
10:49:40  <planetmaker> hg qrefresh -m "Bla blah"
10:49:45  <planetmaker> will add the message in hg
10:50:01  <planetmaker> I'm late :-)
10:50:15  <Zuu> Alberth: will root[-1] blow up if root is empty?
10:50:18  <Alberth> it's not even 1 pm :)
10:50:53  <Zuu> I would guess it would throw an invalid index exception or something like that. Which is fine.
10:51:06  <Alberth>  <-- yes it does :)
10:51:50  <Zuu> root is supposed to be non-empty in all cases where this method is used. So throwing an exception is fine. Much better than returning a path that refers to the root.
10:51:50  <Alberth> which is why I said to test for  len(root) > 0   first
10:52:28  <Alberth> Zuu: ok, I didn't know that, and there is no doc to tell me
10:53:15  <Zuu> Anyway, adding a check may be good. But yes, there is no docs in musa which is why it make it a bit hard to get into. :-)
10:54:26  <Zuu> Ok, I've added "len(root) == 0 or " infront of the root[-1] check.
10:57:59  <Alberth> this is how you produce non-maintainable software, confusion about invariants cause adding checks that may be needed :)
10:58:21  <Zuu> agreed
10:58:27  <Alberth> filename = find_file_in_list(tar.getnames(), [".png", ".bmp"])  versus    scn = find_file_in_list(tar.getnames(), ".scn")
10:59:06  <Alberth> something goes wrong there, or the function does weird magic with its arguments, which is non-pythonic
10:59:28  <Alberth> in 80, but not in your current changes
10:59:31  <Zuu> The method accepts either a list or a string argument.
10:59:46  <Alberth> and how does it know what you give it?
10:59:56  <Zuu> You read the source code of the method
11:00:29  <Zuu> The metod will throw an exception if there are != 1 file found. Thus just iterating a list and call it multiple times is not possible.
11:01:28  <Zuu> Alberth: IIRC it tries to iterate the arg and if that fails, it uses it as a string.
11:01:43  <Alberth> but strings will also iterate
11:02:15  <Alberth>
11:03:17  <Alberth> in general, python says you should know what you get, trying to be smart is bad form.
11:03:22  <Zuu> It was the other way around. It first tries to use it as a string.
11:03:23  <Alberth> but it's not in your changes
11:03:23  <Zuu>
11:04:00  <Alberth> ugh!
11:04:21  <Alberth> bare excepts!
11:04:29  <Alberth> argument value magic!
11:06:27  <Alberth> well, back to the patch in that file, your hunk @@ -296,10 +325,17 @@ and @@ -318,10 +354,17 @@  seems very duplicate
11:06:37  <Alberth> can that be merged in a useful way?
11:08:27  <Zuu> The original intention seemed to be that there is a package_<content> and a validate_<content>. Because all 4 script based content use almost the same logic, I created package_scirpt and validate_script. The same could be done for heightmap and scenario.
11:09:36  <Zuu> Although another convention is that for content that is just one file, 'filename' is not used, but rather the file extension. That is why it says 'scn' in the scenario method.
11:10:27  <Alberth> you're the expert here :)
11:10:59  <Alberth> 90     +#			os.remove(   <-- left-over debug thingie ?
11:11:27  <Zuu> But yes, it would be possible to add package_hm_scn() and validate_hm_scn(), and now that the methods grow beyond a few lines, it might be time to do that. Before it didn't seem worth it.
11:11:44  <Zuu> <Alberth> 90     +#   os.remove(   <-- left-over debug thingie ?   <---- Yes
11:12:05  <Zuu> For getting hold of the tar file created by musa, so that I can analyze it. Will remove that change.
11:16:08  <Alberth> Well, that's what I saw in your patches (and somewhat outside them). I don't have enough context to understand what change you are actually making :(
11:17:08  <Zuu> I'll split out the scenario and hegihtmap package/validate code into a separate method. The amount of arguments to a such generic method is quite few and the code reduction is quite good.
11:17:45  <Zuu> I'll pass [".scn"] instead of magic string/array detection.
11:18:05  <Alberth> I would add a lot of documentation probably, and it looks like the entire code could use a review and a refactoring sweep
11:19:08  <Alberth> (01:20:08 PM) Zuu: I'll pass [".scn"] instead of magic string/array detection.  <-- such changes are mostly useless unless you fixate the interface first by documenting the decision
11:19:38  <Alberth> otherwise nobody is going to understand what to do and not to do
11:19:59  <Alberth> including you, in a few months :)
11:20:14  <Zuu> well, it will remove some magic.
11:20:55  <Alberth> only if you de-magicize the function itself, and fix all calls
11:21:36  <Zuu> I will see if there are any other calls. If there are, I'll leave it as is. But if these are the only calls, then that could be changed now?
11:22:05  <Alberth> as another commit, sure
11:22:38  <Alberth> reducing code magic is always good :)
11:23:22  <Alberth> but since the magic was already there, I would expect some use of it
11:24:04  <Zuu> I think it was added to be able to validate/package heightmaps which have multiple accepted extensions for its single file.
11:24:11  * Zuu hides
11:27:48  *** ntoskrnl has joined
11:27:49  <Alberth> :)
11:31:09  <Zuu> 80 updated so that code duplication is now reduced.
11:31:20  <Zuu> un-magic is added to a new patch 100:
11:46:58  *** Belugas has quit IRC
11:47:33  *** fonsinchen has quit IRC
11:52:34  <Alberth> nice :)
11:54:18  <Alberth> +	return package_hm_scen(tar, tar_path, files, uniqueid, title, ".scn")  <-- [".scn"]  ?
11:55:27  <Alberth> 2 times
11:58:19  *** frosch123 has joined
11:58:19  *** ChanServ sets mode: +v frosch123
12:02:22  <Zuu> Alberth: In 80 or 100?
12:02:42  <Alberth> 80
12:03:19  <Zuu> 80 doesn't have unmagicification.
12:03:42  <Zuu> 100 removes string/list magic
12:04:35  <Zuu> So in 80, it uses ".scn" when it is just one extension.
12:12:53  <Alberth> ok
12:26:12  *** Belugas has joined
12:26:13  *** ChanServ sets mode: +v Belugas
12:30:06  *** fonsinchen has joined
12:30:06  *** ChanServ sets mode: +v fonsinchen
12:46:03  <frosch123> fonsinchen: isn't the behaviour of StationCargoList::StoredCount and TotalCount inverted?
12:46:39  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r25179 || Logs: || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking"
12:47:52  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r25180 || Logs: || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking"
12:51:17  <fonsinchen> Why do you think it is?
12:51:35  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r25181 || Logs: || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking"
12:51:46  <fonsinchen> StoredCount is the stuff that's "really" in the list. TotalCount includes the stuff that "hovers" between two lists.
12:52:14  <frosch123> for vehicles StoredCount tells what is in the vehicle, which excludes stuff from reservation
12:52:40  <frosch123> if storedcount should also tell for stations what is really there, it has to add what is reservered but still at the station
12:53:31  <fonsinchen> That's one possible point of view. Then there is no "TotalCount" for stations, though.
12:53:59  <fonsinchen> The other possible point of view is that reserved cargo belongs to neither cargo list
12:54:19  <fonsinchen> You can explicitly include reserved cargo in either of them by calling TotalCount, though.
12:54:47  <frosch123> well, i thought totalcount would be used in all loading/reservation code, and storedcount in all other places of the code
12:55:01  <fonsinchen> That's not what I thought.
12:55:16  <frosch123> ok :)
12:55:28  <fonsinchen> I thought more along the lines of "what does it mean", not "where is it used".
12:55:46  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r25182 || Logs: || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking"
12:56:05  <frosch123> so, for you StoredCount is always the lower number
12:56:13  <fonsinchen> yes
12:56:53  <fonsinchen> The naming is not set in stone but having a generic Count() seemed confusing, as you've already pointed out.
12:57:27  <frosch123> StoredCount and TotalCount is nice for vehicles. but i don't think StoredCount nails it for stations
12:58:10  <frosch123> maybe StationList::AvailableCount = lower count, and StationList::StoredCount = higher count?
12:59:19  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r25183 || Logs: || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking"
12:59:58  <fonsinchen> Maybe. A problem might be that StoredCount excludes the reservations for vehicles, but includes them for stations.
13:00:34  <fonsinchen> Different semantics for the same method name are probably not such a good idea.
13:00:36  <frosch123> yeah, but that's the case for "stuff which is really there" :)
13:01:09  <frosch123> and i don't thnik the semantics are really different
13:01:55  <frosch123> "stored" sounds to me like it does not consider reservations
13:02:05  <fonsinchen> I think we're having different concepts of "stored".
13:02:33  <frosch123> well, PhysicalyStoredCount then?
13:02:37  <fonsinchen> I was thinking "stored" as in "somewhere in the warehouses" of the station or "inside the waggon"
13:02:58  <fonsinchen> I wasn't thinking of the technical term "stored in memory".
13:03:09  <frosch123> well, i am thinking the same
13:03:31  <frosch123> just that i consider cargo to be either at station or on the vehicle, while you consider stuff in between as neither on station or vehicle
13:05:07  <fonsinchen> What about AvailableCount and TotalCount for stations
13:05:42  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r25184 || Logs: || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking"
13:06:22  <frosch123> yeah,  i guess that works for both of us
13:06:48  <frosch123> ok, i'll replace that locally here then
13:07:04  <fonsinchen> Is there maybe a common word for "Available" and "Stored" we can agree on? Then we have only two method names, not 3.
13:07:38  <frosch123> no, that would return to what was there before
13:07:56  <frosch123> i don't think there is any relation between the two station functions and the two vehicle functions
13:08:07  <frosch123> they are different things, so they shall have different names
13:08:14  <fonsinchen> Well, then let's stick with what we have now.
13:17:48  <planetmaker> hm, musa is nicely hidden. I guess we should add something like ?
13:18:57  <frosch123> who should use that?
13:19:06  <frosch123> we are not building exe files :)
13:20:31  <planetmaker> well. We might want to do that actually, similarily to how devzone makes nmlc.exe
13:20:48  <planetmaker> or at least providing an easy download bundle
13:21:05  <planetmaker> which allows to install that python package
13:21:38  <Zuu> planetmaker: I plan to post to the musa thread once I have got uploading of other stuff than base graphics to work.
13:22:14  <Zuu> However, before I've actually verified that my changes work, I think it is to early to post there.
13:22:19  <planetmaker> not meant to rush you in any way.
13:22:37  <planetmaker> I just wondered how people actually would *know* about it other than a well-hidden forum thread
13:23:30  <Zuu> It could be included in the bananas FAQ for authors as an answer to do certain things not supported in the Web UI.
13:24:06  <frosch123> that sounds like a good spot :)
13:24:15  <planetmaker> agreed. That's a good spot
13:24:31  <Zuu> As it is a python script, you still need to get and install python. Eg. most programmer type of people may already have Python, but random windows user will not have python installed by mistake.
13:25:10  <Zuu> So I wonder if it is useful to offer as a zip/tar download.
13:39:45  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r25185 || Logs: || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking"
13:39:45  <Alberth> nml uses python too, so for newgrf it's no problem in general
13:41:25  <planetmaker> yes, true. But no convenient download bundle (yet) available
14:36:21  <Zuu> I guess we could use that thing that turn a python script into a .exe to offer musa.exe and depgen.exe.
15:33:50  <fonsinchen> So, what to do about the cargodist merge? Shall we try to do something about it until April 22nd or shall we postpone it to June?
15:34:17  <planetmaker> what's special about April 22nd?
15:34:34  <fonsinchen> I'll be gone from April 22nd to May 30th.
15:34:47  <planetmaker> oh :-)
15:35:01  <planetmaker> I hope it's a fun time off :-)
15:35:07  <frosch123> what package would be the next one to merge? is there some part which is closed in itself?
15:36:23  <fonsinchen> There are some pretty isolated things: multimap, smallmatrix, sharesmap and flowstat
15:36:42  <fonsinchen> However, the most interesting thing to do would be the whole link graph queue
15:37:07  <fonsinchen> Eventually someone has to do it ...
15:37:23  <frosch123> well, i meant something which can be tested in trunk in some useful matter without taking everything
15:39:15  <frosch123> can the linkgraph run only for the smallmap without cargodist?
15:39:23  <frosch123> i.e. just to visualise orders?
15:39:37  <fonsinchen> That may be possible
15:39:41  <frosch123> or would that turn stuff upside down? :p
15:39:44  <fonsinchen> let me check
15:40:07  <frosch123> i don't want to add extra work for you, i just wonder about things which can be tested on their own
15:40:40  <fonsinchen> The problem is that saving and loading of all link graph related stuff is in one commit.
15:40:58  <fonsinchen> So, you cannot pick only parts of it without creating desyncs
15:41:23  <fonsinchen> That can be changed, though
15:41:42  <frosch123> maybe i can word my question diffentely: is the linkgraph always available in the cargodist branch? i.e. also when destinations are disabled in adv. settings?
15:42:48  <fonsinchen> It is
15:43:04  <fonsinchen> It just doesn't plan distribution then
15:43:20  <fonsinchen> ie the link graph jobs don't do anything then
15:46:41  *** Zuu has quit IRC
15:49:32  <fonsinchen> It's probably possible to insert some version of the link graph overlay for the smallmap at an earlier place in the patch queue.
15:49:54  <fonsinchen> It's a lot of work to restructure all the code, though.
15:50:43  <frosch123> so, it does not make sense to do it?
15:51:07  <fonsinchen> I think it doesn't. I've tried to add the code file by file to make it easier to review.
15:51:31  <fonsinchen> Breaking that scheme to insert the overlay at an earlier place doesn't help in the end.
15:58:22  <fonsinchen> I mean, for adding the overlay you need at least linkgraph.h linkgraph.cpp, linkgraph_type.h and linkgraph_base.h
15:59:03  <fonsinchen> Then you need the flowstat structure for stations and the obligatory glue.
16:01:54  <fonsinchen> So you need everything up to 22a413dc, plus part of 84c1373b, cf3002e5, ab44fe67 and 6302ceaf
16:02:28  <fonsinchen> In order to see anything you need to track the link capacities, so you need 9443a8c7
16:03:06  <fonsinchen> and then you need the overlay itself, that is 855bf295 and 4c1559dc
16:03:50  <fonsinchen> In the end that's about half of the code from trunk to 4c1559dc in the current order of commits.
16:04:20  <frosch123> if it is only half, then it does not sound so bad :)
16:06:41  <fonsinchen> OK, I'll try to reorder the commits to insert the smallmap overlay at the earliest possible point.
16:07:06  <frosch123> just don't invest too much work, if it is not feasible that way
16:56:42  <fonsinchen> Branch "frosch" in ist the shortest way from trunk to link graph overlays for the smallmap.
16:57:48  <fonsinchen> It may be possible to make it even shorter by sacrificing the flow stats which aren't used, yet at that point.
16:58:13  <fonsinchen> However, to do that I'd have to change some things in the overlay code itself.
16:58:20  <frosch123> thanks, i'll take a look :)
17:17:55  <fonsinchen> In that branch there is no code to remove stale links or "compress" the time the accumulation has been running.
17:18:20  <fonsinchen> So the links will never disappear and may show outdated information.
17:20:08  <fonsinchen> Those things are added in 50d4ed32 and 4af3973a
17:20:23  <fonsinchen> (in my new branch which isn't released yet ...)
17:21:02  <fonsinchen> (now it is)
17:42:51  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r25186 || Logs: || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking"
17:55:11  *** fonsinchen has quit IRC
18:03:40  *** Ristovski has joined
18:37:32  *** ntoskrnl has quit IRC
20:45:06  *** Zuu has joined
20:45:06  *** ChanServ sets mode: +v Zuu
21:01:44  *** frosch123 has quit IRC
21:28:51  *** Alberth has left
21:29:32  *** Zuu has quit IRC
21:29:56  *** Zuu has joined
21:29:56  *** ChanServ sets mode: +v Zuu
22:39:13  *** Ristovski has quit IRC

Powered by YARRSTE version: svn-trunk