I18n: Use fmt::format() to replace string concatenation in nodes #107260

Merged

Bastien Montagne merged 1 commits from pioverfour/blender:dp_geo_nodes_tooltips_fmt into blender-v3.6-release

2023-06-06 16:27:56 +02:00

Damien Picard commented

2023-04-23 04:13:21 +02:00

Member

The UI code for nodes uses stringstreams a few times to format
strings. In some cases it is fine, but in others it makes localization
hard or impossible to do properly, as explained in #92758.

This commit replaces some stringstreams with fmt::format(), a
library that is already used in other parts of Blender and was
suggested by Hans Goudey (@hooglyboogly) in
D15996.

It appears that node socket tooltips are translated during evaluation, which does not happen in the main thread. This means that BLT_translate() will not allow translation to happen. For now, I have modified it to return true, but a better solution should be found and I don’t know where to look. This occurs in at least another area of Blender, more details in #108206.

The UI code for nodes uses `stringstream`s a few times to format strings. In some cases it is fine, but in others it makes localization hard or impossible to do properly, as explained in #92758. This commit replaces some `stringstream`s with `fmt::format()`, a library that is already used in other parts of Blender and was suggested by Hans Goudey (@hooglyboogly) in [D15996](https://archive.blender.org/developer/differential/0015/0015996/#inline-138440). ----- It appears that node socket tooltips are translated during evaluation, which does not happen in the main thread. This means that `BLT_translate()` will not allow translation to happen. For now, I have modified it to return true, but a better solution should be found and I don’t know where to look. This occurs in at least another area of Blender, more details in #108206.

Damien Picard added the

Nodes & Physics

labels 2023-04-23 04:13:21 +02:00

Damien Picard changed title from ~~18n: Use fmt::format() to replace string concatenation in nodes~~ to WIP: 18n: Use fmt::format() to replace string concatenation in nodes

2023-04-23 04:13:47 +02:00

Damien Picard force-pushed dp_geo_nodes_tooltips_fmt from 85be395a81 to 18dff85c0e

2023-04-24 21:21:22 +02:00

Compare

Damien Picard force-pushed dp_geo_nodes_tooltips_fmt from 18dff85c0e to aaec2c7b69

2023-04-25 15:36:53 +02:00

Compare

Damien Picard force-pushed dp_geo_nodes_tooltips_fmt from aaec2c7b69 to bb38e01688

2023-04-29 00:15:13 +02:00

Compare

Damien Picard referenced this pull request

2023-05-23 21:25:25 +02:00

I18n: translation does not occur when started from a thread which isn’t main #108206

Damien Picard force-pushed dp_geo_nodes_tooltips_fmt from bb38e01688 to 015f915e62

2023-05-26 21:01:08 +02:00

Compare

pioverfour changed target branch from main to blender-v3.6-release

2023-05-26 21:01:19 +02:00

Damien Picard changed title from ~~WIP: 18n: Use fmt::format() to replace string concatenation in nodes~~ to 18n: Use fmt::format() to replace string concatenation in nodes

2023-05-26 21:01:53 +02:00

Damien Picard requested review from Bastien Montagne 2023-05-26 21:03:16 +02:00

Damien Picard commented

2023-05-30 22:08:21 +02:00

Author

Member

If this is merged into 3.6, a cleanup will likely need be done when merging into main, to remove #define FMT_HEADER_ONLY, after 0d38fa2422.

If this is merged into 3.6, a cleanup will likely need be done when merging into main, to remove `#define FMT_HEADER_ONLY`, after 0d38fa2422.

Bastien Montagne approved these changes 2023-06-05 18:13:43 +02:00

Bastien Montagne commented

2023-06-05 18:13:55 +02:00

Owner

@blender-bot build

@blender-bot build

Damien Picard changed title from ~~18n: Use fmt::format() to replace string concatenation in nodes~~ to I18n: Use fmt::format() to replace string concatenation in nodes

2023-06-05 21:45:23 +02:00

Bastien Montagne referenced this issue from a commit

2023-06-06 16:27:55 +02:00

18n: Use fmt::format() to replace string concatenation in nodes

Bastien Montagne merged commit d95486af95 into blender-v3.6-release

2023-06-06 16:27:56 +02:00

Bastien Montagne deleted branch dp_geo_nodes_tooltips_fmt

2023-06-06 16:27:57 +02:00

Campbell Barton commented

2023-06-07 05:08:01 +02:00

Owner

This commit comments out threading which wasn't mentioned in the commit log (although I see it's mentioned in the PR).
Now the threading line is left in but commented without any notes for why it's there. In future it please include changes to multi-threaded behavior in the commit log too, and note why commented code is left in.

This commit comments out threading which wasn't mentioned in the commit log (although I see it's mentioned in the PR). Now the threading line is left in but commented without any notes for why it's there. In future it please include changes to multi-threaded behavior in the commit log too, and note why commented code is left in.

Damien Picard commented

2023-06-07 11:21:43 +02:00

Author

Member

In future it please include changes to multi-threaded behavior in the commit log too, and note why commented code is left in.

All right, I understand. If it's too much of an issue right now, this part of the commit can be reverted and tackled separately.

Otherwise I still plan to look into it some more as part of #108206, if only to clean up and comment.

> In future it please include changes to multi-threaded behavior in the commit log too, and note why commented code is left in. All right, I understand. If it's too much of an issue right now, this part of the commit can be reverted and tackled separately. Otherwise I still plan to look into it some more as part of #108206, if only to clean up and comment.

Bastien Montagne commented

2023-06-07 11:28:39 +02:00

Owner

@pioverfour I completely missed this somehow, will revert that part of the change for now indeed.

Afair, boost translation mechanism is not threadsafe at all, which is why it was disabled in any thread but the main one.

@pioverfour I completely missed this somehow, will revert that part of the change for now indeed. Afair, boost translation mechanism is not threadsafe at all, which is why it was disabled in any thread but the main one.

Bastien Montagne commented

2023-06-07 11:48:03 +02:00

Owner

Reverted threading change in 7ceb33e06e.

Reverted threading change in 7ceb33e06e.

Damien Picard commented

2023-06-07 14:03:52 +02:00

Author

Member

Afair, boost translation mechanism is not threadsafe at all, which is why it was disabled in any thread but the main one.

That's what I figured, I wrote about what I found in the other issue. To be honest I was a bit surprised to see this merged! 😅

Do you have suggestions on the best course of action? I can't reproduce the crashes, so I don't know how to determine if the boost issue is still a thing.

> Afair, boost translation mechanism is not threadsafe at all, which is why it was disabled in any thread but the main one. That's what I figured, I wrote about what I found in the other issue. To be honest I was a bit surprised to see this merged! 😅 Do you have suggestions on the best course of action? I can't reproduce the crashes, so I don't know how to determine if the boost issue is still a thing.

Bastien Montagne commented

2023-06-09 14:34:49 +02:00

Owner

@pioverfour tbh I have a hard time understanding why some UI-related string should be handled in a threaded context. Feels like a design issue in the node system...

@pioverfour tbh I have a hard time understanding why some UI-related string should be handled in a threaded context. Feels like a design issue in the node system...

Iliya Katushenock commented

2023-06-09 14:37:08 +02:00

Member

This PR affected on building of internal field names in geometry node evaluator (multithreaded).

This PR affected on building of internal field names in geometry node evaluator (multithreaded).

Bastien Montagne commented

2023-06-09 14:56:03 +02:00

Owner

My problem is once again: why on earth does UI names have to be 'built during nodetree evaluation'? This does not sounds normal to me, at all.

My problem is once again: why on earth does UI names have to be 'built during nodetree evaluation'? This does not sounds normal to me, at all.

Damien Picard commented

2023-06-09 15:05:08 +02:00

Author

Member

In D15996, @HooglyBoogly wrote:

The fact that we translate these strings during evaluation is bad actually, it would be good to avoid that. They should only be translated when we display them in the UI.

To do that we would need a better way to make inspection strings for fields, but that should be doable.

… so I guess this can be considered a known issue.

In [D15996](https://archive.blender.org/developer/differential/0015/0015996/#437415), @HooglyBoogly wrote: > The fact that we translate these strings during evaluation is bad actually, it would be good to avoid that. They should only be translated when we display them in the UI. > To do that we would need a better way to make inspection strings for fields, but that should be doable. … so I guess this can be considered a known issue.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107260

No description provided.