New reading scheme for vbsp, vvis, vrad #356
Conversation
Since standard hammer doesnt support color text, spotting an Error/Warning becomes difficult, so for every Error/Warning that the compilers spells there is going to be an 8 character tabulation for easy spotting. This rule applies for every Error/Warning found in the compilers. Also for hammer++ users this new format combine with colors makes spotting these things very easy and clear. |
|
this PR should be alright, all the useless files/modifications have been removed |
|
i closed the PR by accident, sorry |
|
How should i resolve the conflict in vscript_squirrel.cpp? changing the code to develop? |
The conflict was caused by the recently merged save/restore rewrite. Since I was responsible for merging that and causing this conflict, I took the initiative to resolve the conflict for you, although I think a number of the messages you changed in that file were removed by the rewrite. |
Okay, i will do a PR adding again the modifications from the overwritten file. |
|
I checked the "overwritten file" and all the Error("\t Warning("\t are found in the vscript_squirrel.cpp so it is good to go. Is PR is able to merge right now. |
|
I didn't test it in game, even if it worked it would mess up the scheme in the console, to acknowledge this i will use the next format (only in the shared code between the tooling and game): #if defined(MAPBASE) && defined(SDK_TOOLS) // For the tooling, we use a different printing format
Warning("\tlight has _fifty_percent_distance of %f but no zero_percent_distance\n", d50); //tools
#else
Warning("light has _fifty_percent_distance of %f but no zero_percent_distance\n", d50); //game
#endifThe SDK_TOOLS preprocessor directive will be included in the tooling VPC scripts. |
|
Now, VBSP, VVIS, and VRAD will use the SDK_TOOLS preprocessor directive to enable specific features from the shared code between the game and the tools. Since the tools do not link against tier1, there is no need for a separate build for them. However, mathlib does require this adjustment. VBSP, VVIS, and VRAD will now link against mathlib_sdktools.lib instead of mathlib.lib. See the new VPC file (mathlib_sdktools.vpc), which includes everything from mathlib with an additional SDK_TOOLS preprocessor directive. This is now ready to merge since there is no contamination between the game and tools code. |
There was a problem hiding this comment.
My input on the colours from what is visible on the screenshots is that it is too colourful without a purpose. I don't think successful actions ("done") should be highlighted, they are expected. User input (files) and input data (light count, texinfo etc) can be coloured, though perhaps not as dark as they are there. Colouring each character of RGB is gimmicky and childish, it doesn't provide any value. I didn't check what every compiler output looks like, but think about what information needs emphasis and how it will improve mapper experience.
The overeaching output edits seem completely useless. The blind search & replace in even debug comments doesn't give me confidence that you knowingly applied them. There isn't even any tabs visible in the screenshots you shared apart from a couple of compiler specific lines, so why edit irrelevant files? Instead of adding horizontal space to every output, it might be better to print more to highlight.
Error is a terminating call, they don't even need any formatting at all since they will be the last output.
I don't think text such as **** leaked **** and AREAPORTAL/MAP LEAK should be changed, they have been the same for over 20 years, changing them will only make troubleshooting more confusing.
I don't think usage of arrows (->) instead of colons (:) is appropriate for what the text is trying to convey. Again, seems like a change for change's sake.
I don't understand the comments on StandartColorFormat files. Besides being duplicated between header and code files, why mention ANSI at all? Does ColorSpewMessage not work for all colours, what works then, why is there colours declared that don't work? You have VT enabling code commented out in a couple of places, if colours don't work without it, why is it commented out; if they work without it, why is it there?
"Standard" in "StandartColorFormat" file name is is misspelt.
Hammer++ cmd system doesn't fully support 8-bit RGB colors, so I am limited to using colors that contain only two primary components. For example, 255,128,128 wouldn't work, but 255,128,0 would, That's why the colors appear 'strong'. To acknowledge this issue, I can add a command-line option to disable color highlighting: "-NoColorHighlighting". The RGB color format is meant to be prettier, but I will remove it if it becomes an inconvenience or is disliked.
Could you explain more your point? I dont understand it fully.
Even though is a terminating call is easier to spot in very large .log files with this new format.
The change will be reverted then.
This change was made because all the paths are referenced with
Valve doesn't use ANSI codes; they use ColorSpewMessage with the Color class. So, what I did was provide a 1-to-1 equivalent to Valve's format using all the ANSI colors, in case Hammer++ fixes this issue in the future or if someone implements a Windows-specific function to address it.
Will be fixed. |
|
Changes:
Note: These changes do not apply to VMPI, as no one actively uses it. |
I'm apprehensive about this change. While it provides the benefit of better readability, newer mappers who run into compiler errors may use the Interlopers error checker as a debugger, which can fail to read some of the newly-formatted errors. An example is shown below, where the old format string can be read and produces a search result: However, with your proposed format string, the error doesn't get flagged in the compile checker: The error checker has been a useful tool for many people, and I don't think we should expect newer mappers to be able to trawl through the compile log to find out what problems exist with their compile, nor should we expect Interlopers' error checker to be able to parse these new error strings. |
|
Your concern is reasonable. These changes to the compiler's printing format should be documented in the Mapbase wiki (https://github.com/mapbase-source/source-sdk-2013/wiki/Map-Compilers). This should also be one of the main points in the announcement for Mapbase 8.0 since it is a big change. While new mappers may initially struggle with the new reading format in some cases, this can be easily addressed by providing proper documentation in the Mapbase wiki. Additionally, adding a link to the wiki in the tools description would help guide users to the information found in the wiki. One possible fix would be to add a command-line option like |
New printing formatting (VBSP, VVIS, VRAD)Color sheme:-Green: Process successfully terminated. Printing scheme:
|
|
Concerns about excessive and irrelevant edits weren't addressed. I'm not convinced of the necessity of format changes. New "done" messages seem to be placed blindly and they don't time processes. And I still don't know what "pc" is supposed to stand for here, it doesn't sound like you do either. I have no further comments on this, I don't really care. |
What changes havent been adressed in the commennt "New printing formatting (VBSP, VVIS, VRAD)"? All the new format and changes are explain above.
Many changes are done just for better reading, in other words so the cmd/log doesnt look like garbage. Example old format VBSP: Why
done() is placed after whatever function is complete. Regarding the timing, many functions execute so quickly that adding a timer wouldn't be useful, as it would often display 0 seconds. To maintain consistency between functions that take varying amounts of time to compute, those that complete almost instantly will call done(0) by default. This prevents large discrepancies in the scheme format.
I have explain in "New printing formatting (VBSP, VVIS, VRAD)" and other comment what does.
Lets follow the new
End result:
It is fine, you dont need to respond to the comment, thanks for your review. |
|
In the Mapbase Discord, I was asked to give my opinions regarding this PR. Take my input with a grain of salt, as this is my first contribution of any kind to this project's development. Generally speaking, when you're doing graphics design for text, you should use as few colors as possible to communicate the purpose of your text, and you should never use full-saturation colors unless the information is very important, and definitely not multiple of them in one message, because this is very tiring to the eyes. Remember: Human vision relies primarily on brightness, not color. There also doesn't seem to be much care put into the color psychology of which shades are used for specific purposes ... why did you choose magenta to represent numbers? Since your palette seems to be extremely limited, however, I'll let these things pass. My main issue with the new reading scheme is this: regardless of whether you're a skilled mapper or a beginner, you're not meant to pay active attention to the contents of the compile log, which renders most of these additions trivial by default. Especially these days, where even Walmart's e-waste laptops have more than 4 cores, you're hard-pressed to spend any more than a few seconds in VBSP, and a few minutes in VVIS (with a functionally-optimized map). Most time in VRAD is spent bouncing light, which is a simple progress "bar". Colorizing the compile output adds visual clutter, a lot of which does not convey any new information, to an already cluttered command line. It's probably better for readability to place brackets around numbers, but if they're flying past, the mapper won't be able to read them regardless. And it's very rarely important for the mapper to do so. The new build target output is completely unnecessary, as any end user executing the program will know said target beforehand if the tools were packaged and named properly. It also does not follow any established convention, e.g. Clang's target triples. Most other changes regard basic formatting, which I'm ambivalent towards. This all just seems like unnecessary nipping and tucking, without any real substance. If this PR were merged in full into develop, I would absolutely turn off the colorized output by default, as I would be worried about missing a warning or error. In my opinion, it would be far more helpful for this project to drastically cut down the output of all tools into only the most essential/time-consuming steps of the compile - a "minimal" or "quiet" mode, if you will. However, even that would only be of marginal usefulness. |
Your proposition of "provide documentation" is vague; what kind of documentation do you think should be added? Telling users that one of their previously-existing workflows is now broken (especially for a seemingly tiny change) without providing a fix or workaround isn't a great user experience. The issue isn't the reading format itself, it's that a tool that used to work for almost 2 decades suddenly doesn't work anymore. A formatting change shouldn't need to break anything like that. I agree with you that the command-line switch is unnecessary - I don't see a reason to have two different representations of the same compile log under a command line option. However, if a command-line option is needed to fix something, especially for a formatting change, I think the change has an issue. |
You were right, now there are two separete color schemes. By default, the tools will use a non-saturated one.
It depends on what type of user you are. A normal user who doesn’t heavily use engine resources (models, brushes, luxel scale 16, etc.) won’t see significant benefits from these changes, as they typically don’t pay much attention to the command-line tools and rarely encounter crashes or errors. However, for users who push the engine’s limits (like myself), crashes are frequent, making error tracing tedious. The tools often fail to clearly indicate where a process starts or ends, and their verbosity is limited. With this new scheme, these issues are significantly reduced for such users. Regarding the new headers in the tools, these are minor changes introduced mainly due to the new x64 engine branch. Having these headers helps a bit differentiate crashes and errors between x86 and x64 builds, making it easier to fix the crash. However, since these cases are rare and have minimal impact, 99.9% of users won’t be affected and also mapbase doesnt support at the moment the new x64 engine branch. I can remove this new header format if it becomes an issue, the format that i did use comes from source 2 tools.
This can be implemented if there is enough demand for it. |
Since there is a significant change in the tools, these changes should be documented in the Mapbase wiki in case users want to check the new format Mapbase uses and to maintain a log of the changes. The documentation will cover:
How many users will actually use the Interlopers error checker? It is mostly an obsolete tool now, after two decades, most errors can be easily found on the web with more detailed explanations. If users don't know what an error means, they will likely search for it online (paste the error in the browser), where they can find more comprehensive information than what the Interlopers error checker provides. |
|
I've been mostly silent on this pull request because others already had issues to bring up and I wanted to wait until most of the dust cleared before making a decision. Mapbase has taught me something that I think is really relevant here: When it comes to a feature that people use regularly, a change is inherently bad until it is proven to be good. Even if a feature is badly written or difficult to use, there is normally no risk in leaving it alone compared to trying to fix it. The burden is on you to prove that the change will actually improve the feature. Except this isn't just changing a feature. This is giving the tools a complete makeover and duplicating mathlib to pursue that. That's already asking for a lot. While I agree that the way many messages are printed in the compile tools leaves much to be desired, I don't think this is a good solution to that overall and I can't see this feature being brought to a point where it's acceptable to everyone who uses Mapbase. Even if it has a colorless mode or if the changes were disabled by default, this makeover is still making the same changes to the same code, and even if most users don't see it, that is vulnerable to issues in and of itself. Personally, I would recommend splitting this off into its own fork of the compile tools rather than making it a direct part of Mapbase. I think you would have an easier time convincing people to use it that way. |
(Describe your PR here and then fill out the checklist at the bottom)
This is a PR that aims to make the compile tools more easier to read, both for standart hammer user and hammer++.
Changes:
Examples:
Stock Hammer (new scheme vbsp)
Stock Hammer (old scheme vbsp)
Stock Hammer++ (new scheme vbsp)
Stock Hammer++ (old scheme vbsp)
Stock Hammer++ (new scheme vvis)
Stock Hammer++ (old scheme vvis)
Stock Hammer++ (new scheme vrad)
Stock Hammer++ (old scheme vrad)
Does this PR close any issues?
PR Checklist
developbranch OR targets another branch with a specific goal in mind