search

apocalyptech / eschalon_utils

Eschalon Books I, II, and III Character and Map Editors
http://apocalyptech.com/eschalon/
GNU General Public License v2.0
8 stars 3 forks source link

Global map files in datapak have a loadhook of 1, figure out if we should, too. #62

Open apocalyptech opened 10 years ago

apocalyptech commented 10 years ago

Should we only set loadhook to 2 when loading/saving savefile games? I think that it probably doesn't matter since the loadhook-dependant fields are only loaded for savegames in general, but we could use some more testing to make sure that setting it to 2 doesn't hurt anything.

(Relatedly, this currently has the side-effect that when we load a datapak map and then save it out, the files will not be identical.)

elliotkendallUCSF commented 10 years ago

Hm, good question. I would lean towards retaining what's already in the file in the absence of a compelling argument otherwise.

grimreaper commented 7 years ago

As far as I could tell we already do this with the exception of book III. There we explicitly set it to '2' 1183 # Override the parent class - without this B3 maps won't load 1184 self.loadhook = 2

S>1261 # This isn't really proper but any Book 3 map we load really does need S>1262 # a version of 0.992 and a loadhook of 2. Override them here, in case we 1263 # loaded a map which was written by a buggier older version of this 1264 # utility which might not have set these correctly. 1265 self.version = '0.992' 1266 self.loadhook = 2

Am I missing something?

apocalyptech commented 7 years ago

I'm afraid my memory's fairly fuzzy about what was going on here; I assume that I must've been just talking about Book 3 files, since as you mention, it does look like that's the only place where we're potentially overriding it.

I dunno, it's probably not really important if nobody's noticed problems with it; our current behavior is probably acceptable. When I was working on this actively, I just tended to be pretty paranoid about making changes inadvertently - generally I didn't want to make any changes which I didn't fully understand in the file.

Relatedly, btw, that's actually why I'd historically not actually put limits on our GtkSpinButtons, even when I was pretty sure about their valid ranges - just in case some map somewhere had a value outside what I thought, loading that map cell into the dialog would make the value "snap" to the range specified on the SpinButton and silently cause a change to the file that the user might not be aware of.

Something to keep in mind, though in the end it's probably just me being paranoid.

In re: this particular issue here, I'm guessing I could probably just close it out.

grimreaper commented 7 years ago

Makes sense. Most of the older issues look like GUI specific ones which while I'm not opposed to touching, are things that I'm unlikely to be good at fixing. In general I'd like to add more assertions to file loading so that we never load a file with unknown values, but understand about the silent change concerns.

apocalyptech commented 7 years ago

Ah, THAT idea I absolutely love. Yeah - I'd say go for that. When we think we know a valid range for a value, set proper bounds on its UI element and assert() while loading to make sure. Then if someone does ever come across something out of bounds they could open a bug and it could get sorted.

Anyway, yeah, have at it! Love what you're doing here.