feature(WorldBuilder): add all operation modes to single brush tool #1910

DimitrisKalyvas · 2025-11-25T18:01:11Z

All 4 brush operations can be made using the first brush tool with different hotkeys

…ith scroll wheel

DimitrisKalyvas · 2025-11-25T19:53:15Z

All features added, please review

xezon

First batch of comments.

Overall code looks pretty clean. Was this AI generated? The excessive code comments indicate so. I see some code is similar to adjacent EA code doing similar things.

Unclear to me if all logic is all good. If it is AI generated it needs more intense looking.

xezon · 2025-11-25T19:49:15Z

GeneralsMD/Code/Tools/WorldBuilder/src/WorldBuilder.cpp

 		} else if (0x8000 & ::GetAsyncKeyState(VK_MENU)) {
 			// Alt key gives eyedropper.
 			m_curTool = &m_eyedropperTool;
-		} else if (0x8000 & ::GetAsyncKeyState(VK_CONTROL)) {


Why was this changed? Did VK_CONTROL not work or collide?

As you will also see commented on next line in latest commit, CTRL was the pointer tool. Modern editors use W/E/R hotkeys for Move/Rotate/Scale operation with a gizmo. I may attempt to change this in a future feature by adding gizmo and mentioned operations with hotkeys.

xezon · 2025-11-25T19:52:29Z

GeneralsMD/Code/Tools/WorldBuilder/src/wbview.cpp

+{
+	// Only handle if brush tool is active
+	Tool *pCurTool = WbApp()->getCurTool();
+	if (pCurTool && dynamic_cast<BrushTool*>(pCurTool) != NULL) {


dynamic_cast is not used in this code base

xezon · 2025-11-25T19:55:06Z

GeneralsMD/Code/Tools/WorldBuilder/include/brushoptions.h

+				MIN_SMOOTH_RADIUS=1,
+				MAX_SMOOTH_RADIUS=5,
+				MIN_SMOOTH_RATE=1,
+				MAX_SMOOTH_RATE=10};


This appears to use same values as in BrushTool. Any way to consolidate this?

xezon · 2025-11-25T19:55:21Z

GeneralsMD/Code/Tools/WorldBuilder/include/WorldBuilderView.h

+	// Brush mode hint flicker reduction
+	Int			m_lastBrushMode;							///< Last brush mode drawn (for flicker reduction)
+	CRect		m_lastHintRect;								///< Last hint rectangle drawn (for flicker reduction)
+	CPoint		m_lastHintPos;								///< Last hint position (for flicker reduction)


Indentation

xezon · 2025-11-25T19:58:13Z

GeneralsMD/Code/Tools/WorldBuilder/src/wbview3d.cpp

 	m_highlightTestArt(false),
 	m_showLetterbox(false),
-  m_showSoundCircles(false)
+  m_showSoundCircles(false),


xezon · 2025-11-25T21:18:00Z

GeneralsMD/Code/Tools/WorldBuilder/src/BrushTool.cpp

-	m_prevYIndex = -1;
 	REF_PTR_RELEASE(m_htMapFeatherCopy);
 	m_htMapFeatherCopy = m_htMapEditCopy->duplicate();
+	REF_PTR_RELEASE(m_htMapRateCopy);


Can make this code a bit more readable by rearranging

REF_PTR_RELEASE(m_htMapEditCopy); REF_PTR_RELEASE(m_htMapFeatherCopy); REF_PTR_RELEASE(m_htMapRateCopy); m_htMapEditCopy = pDoc->GetHeightMap()->duplicate(); m_htMapFeatherCopy = m_htMapEditCopy->duplicate(); if (m_activeMode == BRUSH_MODE_SMOOTH) { m_htMapRateCopy = m_htMapEditCopy->duplicate(); resetSmoothRateBuffer(); }

xezon · 2025-11-25T21:24:06Z

GeneralsMD/Code/Tools/WorldBuilder/src/wbview.cpp

+			Int newFeather = currentFeather + delta;
+			// Clamp to reasonable range (0-100)
+			if (newFeather < 0) newFeather = 0;
+			if (newFeather > 100) newFeather = 100;


clamp, several times, then comment is also obsolete.

xezon · 2025-11-25T21:27:35Z

GeneralsMD/Code/Tools/WorldBuilder/src/WorldBuilderView.cpp


+	// Draw brush mode hint (only if in update region or needs update)
+	drawBrushModeHint(&dc, &updateRgn);
+


Excess empty line

xezon · 2025-11-25T22:52:22Z

GeneralsMD/Code/Tools/WorldBuilder/src/WorldBuilderView.cpp

+	if (!pCurTool || dynamic_cast<BrushTool*>(pCurTool) == NULL) {
+		// Not brush tool - clear cache (don't invalidate during paint)
+		if (!m_lastHintRect.IsRectEmpty()) {
+			m_lastHintRect.SetRectEmpty();


This change is sometimes using SetRectEmpty(&m_lastHintRect) and other times m_lastHintRect.SetRectEmpty(). Same with IsRectEmpty. Why is this inconsistent?

xezon · 2025-11-25T22:53:39Z

GeneralsMD/Code/Tools/WorldBuilder/src/wbview3d.cpp

+	m_lastHintPos(-10000, -10000),
+	m_hintDrawnThisFrame(false)
 {
+	::memset(&m_lastHintRect, 0, sizeof(m_lastHintRect));


Can replace with SetRectEmpty?

DimitrisKalyvas · 2025-11-26T01:53:54Z

@xezon thanks for the thorough review! I should have adressed most, if not all comments. Please let me know if you find any more issues.

Yes, a large part of the code is AI generated using Cursor. I've checked that the code compiles and runs as intended in the editor on every commited code. The excessive comments are definitely from this, and I went ahead and manually removed a large portion of them. If you find more that need removal please let me know.

ViTeXFTW · 2025-11-26T06:50:00Z

First of all I would like to say that I appriciate the attention to WorldBuilder!

I saw this in the Discord and was not sure how to feel about it.
I believe it is over engineered, it is already pretty easy to switch between the different tools using the hotkeys ("Q" set hight, "W" raise ground and "E" lower ground) and the addition of additional hotkeys (I believe) will just increase complexity / confusion.

However the ability to change the brush size is definitely a welcome change as this has to be done manually with the mouse.

xezon · 2025-11-26T08:45:54Z

Please figure out what needs changing design wise before the code review continues. I am not familiar with world builder tools so cannot easily judge whether these features make sense. I would need to test it.

ViTeXFTW · 2025-11-26T08:55:31Z

I can try and make a poll in the WB server, but again its hard to debate taste. Some might prefer this and other might like the old way - For now I think it needs some testing to see if it is actually a needed feature or more gimmicky.
I will personally try it out as soon as possible and give it a proper UI/UX review.

DimitrisKalyvas · 2025-11-26T09:22:31Z

@ViTeXFTW Not sure what you mean about Q/W/E hotkeys. Which version of the editor are you referring to? I don't see these keys being used for the modes you mentioned in the files, (and therefore) attempting to switch between these modes in the editor does not work.

Prior to my pull request, W, E and R do not have any standalone binding. CTRL+W views wireframe, CTRL+E views terrain, CTRL+R views contours.

If you are confusing this with the comment I left regarding possible use of W/E/R in the future, that's a different topic.

In any case, these are modernisation efforts. Editors in RTS use the brush tool very often for custom scenario work and I'm going off my personal experience for what makes a fast workflow. Another of these efforts would be the W/E/R hotkeys with gizmo showing on selectable entities, but again that's a completely different (and probably more complex) topic.

If there are any questions or suggestions, happy to discuss them on the Discord.

xezon · 2025-11-26T17:43:21Z

@ViTeXFTW Are you ok with the design of this?

ViTeXFTW · 2025-11-26T19:03:16Z

I haven't tried since I have issues building the project since I updated Windows - Will try it out as soon as I get it up and running. But since there is no hotkey support at all in the vanilla I belive that the first step would be to add support for custom hotkeys which can be assigned by the user - I belive there is a WorldBuilder.ini which could house this data - I am not convinced in the setup of having all tools under the same but that might be because I am biased since I've been using the old community keybinds.

ViTeXFTW · 2025-11-26T19:09:07Z

Saw that I can just download the artifact from GitHub - So am currently testing the WB and will get back with my 2 cents

DimitrisKalyvas · 2025-11-26T19:18:10Z

Adding support for changing keybinds is something to do in the future and I think it's a good feature.

ViTeXFTW · 2025-11-26T19:59:29Z

So - I think overall the change is fine. It does feel like the feature is not entirely complete as you can change the size of the brush with the scroll wheel, but you are not able to easily (without moving the mouse to the field) change the brush height or feather rate.

So I am wandering if there it would be better to split the tool up into 2 "tools" so we don't run out of control keys. If we then add some hotkey feature, to easily select the tools, then I would think the overall flow would still feel fast and dynamic while making sure the user don't have to switch to the old method for some field.

If we go this way then I would suggest keeping the height changing brushes in the tool and add a control key combination for changing terrain height and then make a new which can do the smoothing (Which would be very close to the original, but with some additional hotkeys)

DimitrisKalyvas · 2025-11-26T20:18:00Z

So - I think overall the change is fine. It does feel like the feature is not entirely complete as you can change the size of the brush with the scroll wheel, but you are not able to easily (without moving the mouse to the field) change the brush height or feather rate.

This is currently by design. Now that you mention it, I probably could add hotkeys like A/D to adjust these two values. I'd argue for most editors changing the brush strength/feather rate is not a very useful hotkey (as it generally stays relatively low to avoid aggressive terrain changes), but it would be a rather small addition I think.

So I am wandering if there it would be better to split the tool up into 2 "tools" so we don't run out of control keys. If we then add some hotkey feature, to easily select the tools, then I would think the overall flow would still feel fast and dynamic while making sure the user don't have to switch to the old method for some field.

Would much rather this stays in one tool. Personally I'd even remove the older tools but it goes against the logic of supporting designer's workflows, so if it doesn't cost anything to keep them, I don't see the reason to remove anything.

If we go this way then I would suggest keeping the height changing brushes in the tool and add a control key combination for changing terrain height and then make a new which can do the smoothing (Which would be very close to the original, but with some additional hotkeys)

So make two brushes, one for the height and the other for the smoothing? There's already (more than) two brushes in the current implementation. What I could do would be to check which tool a designer is using and keep the same hotkey. So if you have the height brush, it changes the strength, if not it changes the feather weight.

In any case, I think this is overengineered. Let's add functionality only to one brush, to use hotkeys to change feather and strength values and call it done? And in a future commit I'll try adding functionality to customise hotkeys.

ViTeXFTW · 2025-11-27T05:39:06Z

For now - I don't have any problems with this PR.

However what I forgot yesterday to try out was to create a new map - edit it - save it - and play it. So I would like someone (or me later) to confirm that everything is working as intended.

xezon · 2025-11-27T12:17:25Z

Ok then we can proceed with the code review.

DimitrisKalyvas · 2025-11-27T13:22:32Z

I've already adressed the first round of comments so let me know if you need another round of updates.

DimitrisKalyvas added 3 commits November 25, 2025 19:58

feature(WorldBuilder): add all operation modes to single brush tool

b70665c

feature(worldbuilder): add ability to change feather and brush size w…

02ac220

…ith scroll wheel

feature(worldbuilder): Add on-screen context hint for brush use

a6110ad

xezon reviewed Nov 25, 2025

View reviewed changes

fix(worldbuilder): adress review comments

14299cc

xezon added WorldBuilder Relates to World Builder Enhancement Is new feature or request labels Nov 26, 2025


		// Draw brush mode hint (only if in update region or needs update)
		drawBrushModeHint(&dc, &updateRgn);

feature(WorldBuilder): add all operation modes to single brush tool #1910

Are you sure you want to change the base?

feature(WorldBuilder): add all operation modes to single brush tool #1910

Uh oh!

Conversation

DimitrisKalyvas commented Nov 25, 2025

Uh oh!

DimitrisKalyvas commented Nov 25, 2025

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DimitrisKalyvas commented Nov 26, 2025

Uh oh!

ViTeXFTW commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xezon commented Nov 26, 2025

Uh oh!

ViTeXFTW commented Nov 26, 2025

Uh oh!

DimitrisKalyvas commented Nov 26, 2025

Uh oh!

xezon commented Nov 26, 2025

Uh oh!

ViTeXFTW commented Nov 26, 2025

Uh oh!

ViTeXFTW commented Nov 26, 2025

Uh oh!

DimitrisKalyvas commented Nov 26, 2025

Uh oh!

ViTeXFTW commented Nov 26, 2025

Uh oh!

DimitrisKalyvas commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViTeXFTW commented Nov 27, 2025

Uh oh!

xezon commented Nov 27, 2025

Uh oh!

DimitrisKalyvas commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ViTeXFTW commented Nov 26, 2025 •

edited

Loading

DimitrisKalyvas commented Nov 26, 2025 •

edited

Loading