2.0 Beta 1 Part 3 #722
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: openboe/oboe#722
Reference in New Issue
Block a user
No description provided.
Delete Branch "2.0b1-c"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This was all innocent enough minor fixes until the 3 commits before the last one. Then things got weird.
As I'll be pushing more later let me specify the commits that were major weird stuff...
Remember window positions as preferences
Let the minimap be hidden by other applications in focus
WIP standardize mouse position translation -- The idea of this one was to fix #721 while also making future misalignments between visual selection and actual selection less likely to happen. I have not tested enough to be confident that it did fix the bug.
Fix #61
I'm deep in territory I don't wanna be in. Very yikes.
EDIT: What I mean by that is that I've been making fundamental changes to the input system, which recording and replay can't possibly test. All of these changes cascaded logically out of wanting to fix #721 in a comprehensive, future-proof way. But the changes took a lot of mental wrestling to figure out and I slipped into "Move fast and break things" territory which means I probably made mistakes that could break all kinds of things. I need to go back over these changes very carefully, and if I decide to keep them, it invalidates a lot of the testing I've done, setting me back from approaching a well-tested release candidate.
@@ -132,12 +132,12 @@ rectangle_size_delegate& rectangle_size_delegate::operator-=(int val) {return *this;Could you make these
doubleinstead offloat?@@ -315,2 +324,3 @@const cItem& item = who.items[i_num];bool italic = false;Isn't this the missile graphic number? Is that really a good metric…?
@@ -315,2 +324,3 @@const cItem& item = who.items[i_num];bool italic = false;It's not a great one. The other option I considered was checking variety for whether the item has one of several missile varieties. I went with this because it makes for a simpler check. I want to avoid adding at xml tag that specifies that it should show a quantity when unidentified.
@@ -1733,3 +1817,3 @@return are_done;goto handle_action_return;}Did you test this on Mac? It looks like the kind of code that might need the "input monitoring" permission, but I'm not entirely sure. Maybe it's more lenient with mouse than keyboard.
@@ -43,6 +43,8 @@ const short cDialog::BG_DARK = 5, cDialog::BG_LIGHT = 16;short cDialog::defaultBackground = cDialog::BG_DARK;I really don't like all this minimap handling in
cDialog. It has no business knowing that a minimap even exists.@@ -1733,3 +1817,3 @@return are_done;goto handle_action_return;}Yeah, I wrote it on mac, but I did have the input permissions enabled. So just now I turned off the input permissions to see if it still worked, and it does, so I think it should be fine?
@@ -43,6 +43,8 @@ const short cDialog::BG_DARK = 5, cDialog::BG_LIGHT = 16;short cDialog::defaultBackground = cDialog::BG_DARK;I agree, and I think I've come up with a better way. Instead of have a get_mini_map() delegate it should pass an onLostFocus and onGainedFocus delegate, so the game code can supply all of that logic.
Are you certain
through_sending()wasn't doing anything useful? My current impression of it is that it was intended to slow down the progress of the text buffer if a huge amount of text is added at once, to give you a chance to see it. I don't know if it was actually doing that, though.I removed the functions I removed because I grepped and saw nowhere that would have triggered them being called, either by calling them directly or by setting bools that would result in it happening. That said, I didn't thoroughly look into what those functions were supposed to do, and so it bears looking closer to be more than 90% sure I didn't remove anything that would be good to keep.
I think most special nodes would be previewable, so maybe the preview flag should default to true and be explicitly disabled via
.no_preview()on nodes that can't be previewed.Well, right now each type of node needs to have its preview manually implemented, so inverting that flag would just make the button appear for a bunch of node types that aren't actually previewable until that work gets done. I might be splitting this work into a separate PR and not implementing every type of preview right away.
There is a sensible default for a node that doesn't specify a preview method, though – use msg1 and msg2 for a 2-string dialog.
Good idea, I'll put that in and see how many cases it adequately covers.
2f2f61b6d3dd6a59cf9f2a16e175e683837c8d5c probably should not have changed the filesize, yet the size is more than double. Can you explain that? The files should be saved without needless metadata and in indexed 8-bit format.
That will be for the same reason it happened last time--default GIMP PNG export settings. How did you end up exporting it to get the size down again last time?
I'll fix it soon but another change is coming to the same file, I have someone working on a more visible baby hydra.
I think I unticked every box in the export settings dialog, but I don't remember if I did anything else in addition to that.
If you're rebasing and squashing anyway it's probably better to erase the commit where it inflated.
efab3ee I fixed the size inflation of monst10.png and added a darker baby hydra I had a friend make. We agree it could be prettier but so could all the other sprites--this at least fixes the visibility.
Re: 344a5983dff614d5cd27ff2e90f517c8591f8621
I'm pretty sure I fixed this already…? What's going on here? But 12 is the correct top coordinate when I look at the graphics sheet…
Re: e9bdb9f6d259229061b02d6654e3e7dece0b5afb
Uhh, re-initializing the rects doesn't really seem like the correct approach… I think I'd prefer to have two different rects initialized and select the correct one based on context.
Re: 86bcf2e69faf028852416a91e7aaf49f20d80787
Can we use
\u2026(ie…) instead of...?Re: 264c806440ec0134217b2af8a6cd994b5c537a11
This seems questionable. Changing where just one terrain special is checked seems like it could affect a whole bunch of things that assumed it would be checked in the old place.
First of all, I think we should check what happens in classic BoE in this situation, and stick with that.
Secondly, from my understanding, your bug was that a node that's supposed to run after unlocking the door also runs if you fail to unlock, or choose not to try. That'd be easy to fix in the scenario, by adding an "if terrain this type" node in front of the chain.
Re: d7476326a61c8b825b9dc4074749395abb38fa31
What exactly is more understandable about removing "forced"? I'm not saying it definitely isn't more understandable, but I just don't understand your reasoning here.
This is basically what used to be the "Secret Passage" special node in classic BoE. That is, it allows you to pass through a terrain that is set as impassable.
Re: bd168c9cde19295b8659f49684ebe5456d27f4fa
It looks like your
case TALK_BACKwrongly falls through tocase TALK_ASKif the feature flag is not set or if the dialog history is empty. Also, your two labelled sections at the bottom of the function are indented one level too far.The same source rectangle was hard-coded wrong in more than one place.
I just checked, and the behavior that I went with implementing is what happens in classic BoE (node doesn't happen until you STEP ONTO the space, AFTER it is unlocked).
I'll see if I can keep that behavior while not messing so much with the order of operations.
What gets me about "force allow" is that it feels like an oxymoron to me, especially when the node itself is called "Prevent Action."
You're right. I've just fixed this with a
git commit --amend.Unfortunately that character renders like this:
(Note: in the screenshot it clearly truncates more than it needs to. That's because I hard-coded less space to make sure the truncation would be needed--not because the truncation code is broken.)
In the case of init_shopping_rects() which now responds to changes in the string length of the item cost, re-initializing is required--so it seems fine to me to do it for this one too. I mean, maybe we would want to re-initialize these ones as well, for example, to make it so each rectangle only covers the actual rendered text and not all the extra space. (I don't think that would be better, but it's a plausible example because we do that with LED labels.)
I thought about doing it the other way and felt like that would entail making item_buttons a 3-dimensional array...
[which_item][which_button][which_mode]and that seemed pretty silly (when I first encountered this code with the 2-dimensional arrays it confused me and took some getting used to).Now that I think about it, it feels potentially wrong that special spots would run before special terrain handling in any case whatsoever, not just locked doors. I think I'm going to double-check with the original source release, and maybe test more cases in legacy BoE. But what I'm wondering is whether the whole switch-case shouldn't move to where I moved the lock door case.
EDIT: Looking at the original code, I actually can't figure out why it behaves any differently than what we have now. But it does. I wish I could attach a debugger to the original source code but I'm guessing it could be literally impossible to get it to build.
If the change that introduced the bug happened after OpenBoE adopted version control, maybe git bisect will find the problem but it would still involve testing revisions where building to test would be very, very hard.
I think I figured it out. Just need to test something else in classic BoE.
@@ -1170,13 +1219,25 @@ void handle_talk_node(int which_talk_entry) {save_talk_str1 = s1 >= 0 ? univ.scenario.spec_strs[s1] : "";This stuff is still indented one level too deep (except for
if(false){and}).@@ -1170,13 +1219,25 @@ void handle_talk_node(int which_talk_entry) {save_talk_str1 = s1 >= 0 ? univ.scenario.spec_strs[s1] : "";whoops, sorry.
I don't like this: 1aeac0c1631106d6f07c02cb5fe71cffedf3fc43
If you want an acid damage type, then add an acid damage type. Don't just half add it. And why on earth is it -1? That makes no sense. Also, if you're adding it then it needs its own boom. Maybe as a quick temp graphic you can just invert the magic boom.
Personally, I don't think we really need a separate acid damage type. I don't especially mind having one either, though.
The next ones say 'keep these two last'--I am afraid of changing their values (or any of the values in this enum) because magic number references to them could be anywhere.
I'm still processing it as magic damage so as not to change the game balance--magic resistance has always applied to it, and there are no features anywhere that would add acid resistance. The reason for "half-adding" it now, is mainly to use the intended sound effect and make it easy to add a graphic for it later.
EDIT: Oh, yeah, I can see what I could come up with as a quick temp graphic, yes.
Well, this commit changed SPECIAL from 9 to 8:
d877df5101So that suggests to me that changing it back to 9 shouldn't cause any problems.
Its existence in the enum means you can set acid resistance and whatever other things you can come up with.
I think the correct approach is to handle it like I handled the new races (Vahnatai, Goblin, Skeletal Undead, possibly a few others) – basically, anything that checks for magical also covers acid. So, magic resistance also gives acid resistance, and protection from magic also protects from acid, etc.
Re: 1d3a641ef04904c440a5278ca1cd399905fa8652
For this you probably need to update unit tests and maybe some of the sample scenario files in the test folder.
Oh, and the scenario schema file as well. Would be nice if you can make a new schema for the editor file too…
For a temporary acid boom, I started with this monster graphic from BoA:
I isolated the slime, then scaled it down to fit BoE boom size.
BoA credits two artists, one for the line art, and one for the icons. Andrew Hunter is the icon artist, and already credited as the main artist for BoE. I think somewhere you said we already have Andrew Hunter's permission to use additional graphics he made for Spiderweb games? Anyway, we can always ask to make sure.
Uhh… what? I think using graphics from BoA is fairly questionable – I even left out a bunch of graphics that Mistb0rn created specifically for BoE because of that. Besides that, the graphic doesn't even match the aesthetics of the other booms. That makes it a really bad fit even as a temp graphic.
Maaaybe you could get something useful out of that slime if you mirrored it vertically and squished it in just the right way (not uniformly – the protrusions would need to shrink more than the centre), but I'd much rather take one of the pre-existing booms on that sheet and modify it.
We have Andrew Hunter's permission to use graphics he created for the Exile trilogy. I don't know if that permission extends to graphics created for the Avernum series. I didn't even know he had created those graphics.
I reeeeaaaally don't think it sticks out so horribly as you're saying. Just a reminder that I'm doing my best and trying to make as many improvements as I can over here with limited time to work. I tried fiddling with the pre-existing booms and couldn't come up with anything that felt distinct enough. I do think what I came up with is decent.
EDIT: Not saying no to the criticism, I just feel some of these reactions lately imply what I did is utterly baffling--in an iterative process where things aren't finished, no less.
Also, damaging terrains write out their damage type in terrain.xml as an integer, so making changes to eDamageType values could definitely be destructive.
That should be fixed, but there's no need to worry about it here – there won't be any damaging terrains that do special damage in any scenario.
I think the point of resist magic was improved saving throws, not reduced damage. (Assuming you mean the status effect.) That said, I don't have any problem with changing that, especially considering it does reduce fire and ice damage.
How about "It burns!"? Or something along those lines.
I don't really know if it could be called decent, but I do know that it doesn't really fit in with the others and I don't really like it.
I had a thought that the ideal boom would be a larger version of the acid status icon (since the poison boom and status icon already match). I tried fiddling a bit on making the poison boom be more like the acid status icon, but couldn't come up with anything good.
I truly was baffled when you picked something from Blades of Avernum… I can't think of anything else that counted as truly baffling though.
@@ -1066,6 +1070,58 @@ void readScenarioFromXml(ticpp::Document&& data, cScenario& scenario) {throw xMissingElem("scenario", *reqs.begin(), data.FirstChildElement()->Row(), data.FirstChildElement()->Column(), fname);Please use an
Iterator<Element>for this.Please use an
Iterator<Element>for this.Please put this information as x and y attributes on the
<out-view-state>tag.Please put this information as an attribute on the
<town-view-state>tag.@@ -4147,3 +4153,4 @@showError("Tried to change the attitude of nonexistent monster " + std::to_string(spec.ex1a) + " of 0..." + std::to_string(num_monst));break;}if(spec.ex1b < 0 || spec.ex1b > 3){Using
athere doesn't help anything. It still crashes, just reliably (ie, every time) and sooner (ie, not later on because of data corruption). What's really needed is a manual range check… which means you might as well stick to the[]operator.@@ -1066,6 +1070,58 @@ void readScenarioFromXml(ticpp::Document&& data, cScenario& scenario) {throw xMissingElem("scenario", *reqs.begin(), data.FirstChildElement()->Row(), data.FirstChildElement()->Column(), fname);When I set it up that way,
The loop body never executes. I swear I'm following the exact same pattern as the enclosing loop, but it doesn't work, begin() returns end() even when elem->FirstChildElement() returns the proper result. When I switched to the pointer way it did work, and I just did not care to debug a non-trivial tinyxml bug when I can't find documentation of how the iterator is supposed to work.
@@ -1066,6 +1070,58 @@ void readScenarioFromXml(ticpp::Document&& data, cScenario& scenario) {throw xMissingElem("scenario", *reqs.begin(), data.FirstChildElement()->Row(), data.FirstChildElement()->Column(), fname);Ohhhh. I think I see the problem, and tutorial_ticpp.txt actually does have info on this. Another thing I'd like to make easier for new contributors to find and learn from.
@@ -1066,6 +1070,58 @@ void readScenarioFromXml(ticpp::Document&& data, cScenario& scenario) {throw xMissingElem("scenario", *reqs.begin(), data.FirstChildElement()->Row(), data.FirstChildElement()->Column(), fname);It has to be like this:
Dereferencing the iterator to get the Element, then passing its address to child.begin(). What misled me was that the enclosing loop is calling FirstChildElement() on the document, but when doing that on the inner loop it actually tries to iterate the inner child. This all makes sense to me now.
@@ -1066,6 +1070,58 @@ void readScenarioFromXml(ticpp::Document&& data, cScenario& scenario) {throw xMissingElem("scenario", *reqs.begin(), data.FirstChildElement()->Row(), data.FirstChildElement()->Column(), fname);or instead of
&(*,.Get()Yeah, the ticpp iterator is a bit weird, but there are plenty of working examples in the source code already.