Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make always researched researchitems unpickable. #7628

Merged
merged 5 commits into from
Jun 15, 2018
Merged

make always researched researchitems unpickable. #7628

merged 5 commits into from
Jun 15, 2018

Conversation

Chaosmeister
Copy link
Contributor

@Chaosmeister Chaosmeister commented Jun 4, 2018

This fix makes the always-researched researchitems in the inventionlist un-pick and -movable

It was mentioned in #7601

It now also renders the always-researched items like the vanilla version again.

y -= SCROLLABLE_ROW_HEIGHT;
if (y < 0)
{
if (research_item_is_always_researched(researchItem))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nicer to perform this check only where it's needed. It would make sense for window_editor_inventions_list_get_item_from_scroll_y to return the research item for always researched items too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this is exactly what fixes this behaviour

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but the function loses its meaning now. You should add this check to places where it's needed instead, those being -_scrollmousedown, -_scrollmouseover and -_drag_moved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understood! I'll move it.

Copy link
Contributor Author

@Chaosmeister Chaosmeister Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a double-edged sword:

I just moved the check to the calling functions.
I then recognized, that the check is required in every function that calls either
window_editor_inventions_list_get_item_from_scroll_y_include_seps or
window_editor_inventions_list_get_item_from_scroll_y

So apparently it's always required and should stay in
window_editor_inventions_list_get_item_from_scroll_y
so that new calls to it don't introduce bugs with always-researched items.

We could either risk introduction of bugs by forgetting to check the returnvalue of the _get_item_from functions against is_always_researched or risk bugs by not getting always-researched items at all.

The second bug-source might be mitigated by renaming the getter functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not always required. Look at window_editor_inventions_list_scrollmouseover for example. Right now, because of the check, you don't see the row highlighted when hovering over an always-invented research item. The only place where the check is needed is on window_editor_inventions_list_scrollmousedown (for making it non-pickable) and window_editor_inventions_list_cursor (for setting the hand cursor).

All other calls to these functions (and get_research_item_at which is basically a wrapper) don't need this check. I've tested this locally now, to make extra sure they really aren't needed. If you're okay with it, I can push my changes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say it should not be highlighted etc as well

Copy link
Member

@Broxzier Broxzier Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's how the original games does it too, but it can't hurt to have the missing highlight as extra user feedback. To fix the cursor feedback, the check should be removed from window_editor_inventions_list_cursor though.

Copy link
Contributor Author

@Chaosmeister Chaosmeister Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about window_editor_inventions_list_drag_moved? putting it in there would prevent dropping items between the non-pickables

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a nice thing to do. Maybe add comments too, to explain why some of the checks are there.

@@ -145,6 +145,29 @@ static void DrawTextEllipsisedCompat(rct_drawpixelinfo * dpi, sint32 x, sint32 y
DrawText(dpi, x, y, &_legacyPaint, buffer);
}

static void DrawTextEllipsisedCompatNoFontReset(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does 'No Font Reset' mean?

Copy link
Contributor Author

@Chaosmeister Chaosmeister Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DrawTextEllipsisedCompat always resets _legacyPaint.SpriteBase to FONT_SPRITE_BASE_MEDIUM

This new function uses the requested gCurrentFontSpriteBase;

I didn't want to introduce another parameter so introduced a separate function.

@Gymnasiast
Copy link
Member

This does add quite a lot of code for a bugfix.

@AaronVanGeffen
Copy link
Member

I have submitted a PR with an alternate fix for the issue with inset text: #7638

@AaronVanGeffen
Copy link
Member

With #7638 in, I've rebased this PR on top of develop. @Broxzier's comments have yet to be addressed, though.

{
researchItem = get_research_item_at(x, y);
y += LIST_ROW_HEIGHT;
} while (researchItem != nullptr && research_item_is_always_researched(researchItem));
Copy link
Member

@Broxzier Broxzier Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a bit more work. get_research_item_at does not return nullptr when at the end of the list, but an entry with values like -1 and 0xFF.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you really sure about that, @Broxzier ? get_research_item_at takes window coordinates and can return nullptr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite sure, yes. It returns nullptr when clicked outside of the scroll areas, but when inside the scroll area, it returns the separator.

@AaronVanGeffen
Copy link
Member

Seems to be working well now! Can be merged when CI turns green, as far as I'm concerned.

@Gymnasiast Gymnasiast merged commit 7bd4244 into OpenRCT2:develop Jun 15, 2018
janisozaur added a commit that referenced this pull request Aug 26, 2018
- Feature: [#5993] Ride window prices can now be set via text input.
- Feature: [#6998] Guests now wait for passing vehicles before crossing railway tracks.
- Feature: [#7658] Add option to always use system file browsing window.
- Feature: [#7694] Debug option to visualize paths that the game detects as wide.
- Feature: [#7713] The virtual floor now takes land ownership rights into account.
- Feature: [#7771] Danish translation.
- Feature: [#7797, #7802, #7821, #7830] Add sprite font glyphs for Danish, Norwegian, Russian, Turkish, Catalan and Romanian.
- Feature: [#7848] Add a master volume slider to audio options screen.
- Feature: [#7868] Placing scenery while holding shift now scales appropriately with zoom levels.
- Feature: [#7882] Auto-detect Steam and GOG installations of RCT1.
- Feature: [#7885] Turkish translation.
- Fix: [#3177] Wrong keys displayed in shortcut menu.
- Fix: [#4039] No sprite font glyph for German opening quotation mark.
- Fix: [#5548] platform_get_locale_date_format is not implemented for Linux.
- Fix: [#7204] Object source filters do not work for RCT1, AA and LL.
- Fix: [#7440] Memory leak. All system memory used.
- Fix: [#7462] Guest window goes beyond the map edge on a spiral slide.
- Fix: [#7533] Screenshot is incorrectly named/file is not generated in CJK language.
- Fix: [#7628] Always-researched items can be modified in the inventory list.
- Fix: [#7643] No Money scenarios with funding set to zero.
- Fix: [#7653] Finances money spinner is too narrow for big loans.
- Fix: [#7673] Vehicle names are cut off in invention list.
- Fix: [#7674] Rides show up as random numbers in guest's ride list.
- Fix: [#7678] Crash when loading or starting a new game while having object selection window open.
- Fix: [#7683] 'Arbitrary ride type' dropdown state is shared between windows.
- Fix: [#7697] Some scenery groups in RCT1 saves are never invented.
- Fix: [#7711] Inverted Hairpin Coaster allows building invisible banked pieces.
- Fix: [#7734] Title sequence not included in macOS builds as of 0.2.0 release.
- Fix: [#7756] Steam RCT2 path not correctly checked on macOS and Linux.
- Fix: [#7765] Crash when opening ride list window on Windows Vista.
- Fix: [#7773] Once research has been completed, player is still charged for research.
- Fix: [#7786] Crash when importing a track design.
- Fix: [#7793] Duplicate private keys generated.
- Fix: [#7817] No sprite font glyph for interpunct.
- Fix: [#7823] You can build mazes in pause mode.
- Fix: [#7804] Russian ride descriptions are cut off.
- Fix: [#7872] CJK tooltips are often cut off.
- Fix: [#7895] Import of Mega Park and the RCT1 title music do not work on some RCT1 sources.
- Improved: [#7899] Timestamps in the load/save screen are now displayed using local timezone instead of GMT.
- Improved: [#7918] Better RCT2 detection if both disc and GOG/Steam versions are installed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants