Fixing and improving the Tuning menu. #3151
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 tasks
|
I'm fixing the addonpart conflicts - I figured I need to block the user from selecting conflicting addonparts at all, preferably with some visual markers. This is what I have right now - the graphics are not final and there's a huge lag when opening the menu. To be improved. |
|
Markers are finished:
|
ohlidalp
force-pushed
the
3122_Tuning_glitches
branch
from
6a6ba9d
to
ac7e390
Compare
ohlidalp
added a commit
to ohlidalp/rigs-of-rods
that referenced
this pull request
Apr 24, 2024
The modder can add any number of `addonpart_filename <foo.truck>` directives (case-insensitive). If none are specified, any filename goes. Matching by GUID is still the primary method, this only helps in cases where the GUID is shared by too many trucks (not uncommon). This commit also fixes the GUID filtering being case-sensitive in all cases (trucks, skins, addonparts), even though the search string is always case-insensitive. Cache file format was set to dummy value; will be properly bumped when RigsOfRods#3151 is merged.
Added
|
|
Unfortunately adding flares doesn't seem to work for me: Expected result: RoR_2024-04-26_01-15-41.mp4
|
|
I fixed the EDIT: Another fix - I forgot to update the condition around Finally I fixed the case where the flare used material from the addonpart ZIP - same story as with props and flexbodies, I just forgot to handle it the first time. |
ohlidalp
force-pushed
the
3122_Tuning_glitches
branch
from
2c13dd1
to
d62d108
Compare
ohlidalp
force-pushed
the
3122_Tuning_glitches
branch
from
e802385
to
375eb4e
Compare
|
Added managed materials tweaks: |
ohlidalp
force-pushed
the
3122_Tuning_glitches
branch
from
375eb4e
to
4696d87
Compare
INFO log about adding tweak from addonpart to tuneup was changed from "tweaking *** with params" to "Scheduling tweak for *** with params" to clarify it's not applied yet. INFO log about withdrawing tweak from tuneup due to another addonpart trying to tweak the same element was changed from "conflict of tweaks at ***" to WARNING "Resetting tweaks for *** due to conflict" The idea was that 2 or more addonparts cannot tweak the same element. There is a flaw in this logic though: if there are 3 conflicting addonparts, the second one will reset the first one, but the third one will pass. And so on. Additionally this commit fixes terminology in the logs: addonpart keywords are now called 'directives', not 'elements' ... because elements mean the things actors consist of (wheels/props/flexbodies/nodes etc...).
Broken in 9551d05 - since this commit flexbodies got a dynamic visibility control, exactly the same as props. From user (truck fileformat) perspective, it was the same already but the update pattern was different, and this commit unified it. The FlexbodyDebug UI didn't reflect this however. In addition to fixing the glitch, this commit also unifies code for the visibility handling - same variables, same constants, just different prefixes. Codechanges: * GfxData: cosmetic changes to CAMERA_MODE constants - added typedef * FlexBody: replaced single visibility mode field with _active+_orig fields, same as props have. Also using the same coding style as props do. * GfxActor: cosmetic change - use the new variables in flexbody. * ActorSpawner: set flexbody visibility via the new vars.
This implements proactive scanning for addonpart conflicts when the menu is refreshed. This incurs a big lag when opening the menu and likely a lot of spam in RoR.log, but it works. Currently it only displays red squares as conflict markers when one addonpart is hovered. The original conflict resolution logic is still present in `ResolveUnwantedAndTweakedElements()` - when a conflict is found, tweeaks for that elements are reset. But this is flawed, if there's an odd number of conflicting tweaks, the last one will pass. I'll remove it in next commits.
This removes the huge UI lag on first open of the tuning menu, which was caused by loading all addonparts to evaluate conflicts. But the lag isnt gone - it now happens upon spawning the actor. It could only be fully eliminated by caching the tweaked element IDs directly in `CacheEntry`, something I'll eventually do but not now. Codechanges: * CacheSystem: added dummy TuneupDef with addonpart data to CacheEntry. * AddonPartFileFormat: added silent mode, using the new CacheEntry field. * GameContext: also push MSG_SIM_REFRESH_TUNING_MENU_REQUESTED after spawning new actor and seating player there. Deleted duplicate code in SpawnActor()
Gray X = conflict with installed addonpart. Red [] = conflict with mouse-hovered addonpart (regardless of being installed or not)
The modder can add any number of `addonpart_filename <foo.truck>` directives (case-insensitive). If none are specified, any filename goes. Matching by GUID is still the primary method, this only helps in cases where the GUID is shared by too many trucks (not uncommon). This commit also fixes the GUID filtering being case-sensitive in all cases (trucks, skins, addonparts), even though the search string is always case-insensitive. Cache file format was set to dummy value; will be properly bumped when RigsOfRods#3151 is merged.
This was an exercise in extending the Tuning system. Took 2 hours, 35 minutes (including writing the long HOWTO below).
* An `addonpart_unwanted_flare` directive was added.
* Addonparts now support 'flares' and 'flares2' (flares3 aren't supported).
* Note that tweaks for flares were not added, as adding/removing seems to be enough.
Codechanges (HOWTO add a new feature to the Tuning system [if you're a daring programmer] - changes are ordered to intuitively form the picture):
* ForwardDeclarations.h: added typedef FlareID_t ~ this isn't technically necessary but makes the code more pleasant to read.
* TuneupFileFormat.h: added unwanted/forece_remove/protected flare lists to the TuneupDef document, and helper functions to TuneupUtil. See //comments to understand their meaning. ~ I just duplicated the same code as props already use, just changed the ID type to flares.
* AddonPartFileFormat: added support for defining 'flares' and 'flares2' in the .addonpart file. ~ Since the syntax is the same as in 'truck' fileformat and the output object is also the same, this basically means copypasting `RigDef::Parser::ParseFlaresUnified()` (see file RigDef_Parser.cpp) and modifying it to fit. A slight twist is that I didn't use an unified function but created separate 'flares' and 'flares2' functions. This is because 'flares3' is separate either way.
FIXUP: I forgot to update the condition around `block = keyword;`, causing the parser to interpret all flare lines as whatever was before (flexbodies in my case).
* CacheSystem: added PROTECTED + FORCEREMOVE SET/RESET ModifyProjectRequest types. ~ in RoR, everything important is done via message queue. Tuning changes are done by MSG_EDI_MODIFY_PROJECT_REQUESTED.
* GUI_TopMenubar.cpp: added "Flares" section ~ pretty much a copypaste of existing Flexbodies section code above, just modified to fit: changed the loop to read flares instead of flexbodies, added code to compose name of the flare, and change the ModifyProjectRequestType constants.
* ActorSpawner: if the flare is removed by Tuning system, just create a placeholder. ~ This is the moment of truth. When spawning, the tuning changes must be evaluated, but to maintain consistent lists of elements in the Tuning menu (and other tools), the unwanted/forceRemoved elements are left around as inactive 'placeholders'. In this case, it's a complete flare object, just without the visual flare billboard and light.
* SimData.h: just // comments about tuning
* GfxActor: skip placeholder flares when updating visuals.
* DashBoardManager: added helper function to convert linkID back to the name - needed for the Tuning UI menu to display link name for 'd' (dashboard) flares.
To understand code changes, find previous commit which adds flares to the tuning system - it provides detailed checklist for making such change.
If OPTION_ALLOW_NAKED_STRINGS was used, strings starting like true/false, for example f or t alone, would get discarded.
Of course I could't get it right the first time. The mechanism I added is the same thing that props and flexbodies already use. Search fulltext for `*_rg_override`.
Null pointer access :/ I removed the confusing variable `current_actor` altogether - now there's only `tuning_actor`.
This fixes 55e98ca which introduced `_somemedia_override_rg` fields to props and flexbodies (later followed by flares), alghough there was already a more elegant solution - the `RigDef::Module::origin_addonpart` field used by managedmaterials (created along with the very inception of the AddonPart system in b1f4d23). I couldn't determine why I created this redundant logic in the first place. Either way, This commit unifies the logic around the `origin_addonpart` mechanic.
Another exercise in extending the Tuning system. Took ~5 hours (2 hours just Spawner - resolving texture tweaks was tricky)
* An `addonpart_unwanted_managedmaterial` directive was added.
* An `addonpart_tweak_managedmaterial <name> <type> <media1> <media2> [<media3>]` directive was added.
Unlike all other elements which are referenced by ID, managedmaterials are referenced by name because it must be unique anyway.
Codechanges ~HOWTO add a new feature to the Tuning system [if you're a daring programmer]
changes are ordered to intuitively form the picture
(this is an updated version of the 'flares' guide)
==================================================
* TuneupFileFormat.h:
- added struct TuneupManagedMatTweak ~ same pattern as other tweaks.
- added unwanted/forece_remove/protected flare lists to the TuneupDef document, and helper functions to TuneupUtil. See //comments to understand their meaning.
~ I followed the same pattern as other elements use, except I used plain `std::string` for an ID.
* TuneupFileFormat.cpp:
- implemented all new functions, in same manner as nodes/flexbodies/props already do. No special features.
* AddonPartFileFormat:
- Added ProcessTweaked***() and ProcessUnwanted***() helper functions for managedmats.
- Extended `ResolveUnwantedAndTweakedElements()` to use them.
* CacheSystem:
- added PROTECTED + FORCEREMOVE SET/RESET ModifyProjectRequest types. ~ in RoR, everything important is done via message queue. Tuning changes are done by MSG_EDI_MODIFY_PROJECT_REQUESTED.
~ Unlike other elements which use `subject_id` field (integer), these use `subject` (string) field, see //!< comments.
* GUI_TopMenubar.cpp:
- Added magic constant TUNING_SUBJECTID_USE_NAME to be able to use the helper funcs `DrawTuning{ForceRemove/Protected}Controls` with name (subject) instead of ID (subject_id)
- added "Managed Materials" section ~ pretty much a copypaste of the Exhausts section code above, just modified to fit: changed the loop to read managedmats instead of exhausts, added code to compose name of the flare, and use the new SUBJECTID_USE_NAME constant.
* ActorSpawner:
- created `AssignManagedMaterialTexture()` to resolve Tweaks and check if texture exists - Helper for `ProcessManagedMaterial()`.
- if the managed material is removed by Tuning system, just create a placeholder. ~ This is the moment of truth. When spawning, the tuning changes must be evaluated, but to maintain consistent lists of elements in the Tuning menu (and other tools), the unwanted/forceRemoved elements are left around as inactive 'placeholders'. In this case, it's a clone of "tracks/transred" material from builtin resources.
ohlidalp
force-pushed
the
3122_Tuning_glitches
branch
from
4696d87
to
5e77521
Compare
|
I fixed the remaining 2 glitches:
|
There is a new field in the .tuneup format, 'filename'. It should really be 'associated_filename', but this would be confusing with 'guid' (which should really be 'associated_guid') and changing that would be confusing with .skin format which also uses 'guid' and cannot be changed because #compatibility. C'est la vie.
ohlidalp
force-pushed
the
3122_Tuning_glitches
branch
from
5e77521
to
14cffb5
Compare
Cause: bad loop code in AddonPartFileFormat.cpp, function `RecordAddonpartConflicts()`, which inserted garbage into `_tweaks` arrays. For example if the addonpart only tweaked prop 6, there should be just the item 6 in the set, but it ended up being garbage items 0-5 and valid 6. Why? This is a backwards compatibility quirk of C++: when you ask a `std::set<>` for non-existent element via `[]` brackets, it inserts a dummy element and gives it to you. And because these loops went "from 0 to number of elements", gradually, as repeated conflict resolutions happened, garbage built up. Fixed by looping using iterators which only retrieve actually existing elements.
CuriousMike56
approved these changes
May 28, 2024
ohlidalp
added a commit
that referenced
this pull request
May 28, 2024
The modder can add any number of `addonpart_filename <foo.truck>` directives (case-insensitive). If none are specified, any filename goes. Matching by GUID is still the primary method, this only helps in cases where the GUID is shared by too many trucks (not uncommon). This commit also fixes the GUID filtering being case-sensitive in all cases (trucks, skins, addonparts), even though the search string is always case-insensitive. Cache file format was set to dummy value; will be properly bumped when #3151 is merged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Highlights:
addonpart_filename <foo.truck>to whitelist only some vehicles if GUID is too broad.filename <foo.truck>to filter the "Saved tuneups" list by vehicle filename, since GUID is too broad and some vehicles don't even have one.Changes made so far:
flare/flare2/addonpart_unwanted_flarein .addonpart, protected/forceremoved states in .tuneup, including UI. Tweaks were omitted.addonpart_unwanted_exhaustsin .addonpart, protectd/forceremoved states in .tuneup, including UI. Tweaks and defining new exhausts in .addonpart were omitted.GenericDocumenttokenizer - IfOPTION_ALLOW_NAKED_STRINGSwas used, strings starting liketrue/false, for examplefortalone, would get discarded.Additional discovered issues:
The idea was that 2 or more addonparts cannot tweak the same element. There is a flaw in this logic though: if there are 3 conflicting addonparts, the second one will reset the first one, but the third one will pass. And so on.