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 formspec elements real elements for draw order and clipping #8740

Open
wants to merge 3 commits into
base: master
from

Conversation

@DS-Minetest
Copy link
Contributor

commented Aug 2, 2019

  • Currently the formspec element draw order can't be changed for most elements. This is because they are drawn manually.
    Also the clipping doesn't work correctly for many elements.
  • This PR implements (nearly) all formspec elements as real gui elements and fixes those issues.
  • The changes are taken from #8591 to make that PR simpler. #4844 is related but not fixed. Lists are still drawn at last because changing this is more difficult, but they are clippable.
    Closes #8036.

To do

This PR is a Ready for Review.

How to test

Here is some lua code for testing.
-- edit this formspec,
-- especially change the order of the elements,
-- then compare. also compare to master

local my_formspec =
	"formspec_version[2]".. -- comment this to test backwards compatibility
	"size[10,8]"..
	"real_coordinates[true]"..

	"button[2.5,0.5;1,1;bla;Bla]"..
	"box[0,0;3,2;#00ff00ff]"..
	"tooltip[bla;Blatt;#f00;#000]"..
	"style[button_noclip;noclip=true]"..
	"button[-1,2.5;3,1;button_noclip;Noclip Button]"..
	"style[field_noclip;noclip=true]"..
	"field[-0.5,2.8;2,1;field_noclip;noclip field;some default text]"..
	"box[1,1;1,2;#1111ff]"..
	"bgcolor[;true]"..
	"background[1.5,1;2,2;default_wood.png;true]"..

	-- here are more elements, uncomment and test them
	--~ "scrollbar[7.5,0;0.3,4;vertical;scrbar;0]"..
	--~ "pwdfield[2,2;1,1;baum2;Baum]"..
	--~ "list[current_player;main;4,4;1,5;]"..
	--~ "image[3,1;bubble.png]"..
	--~ "item_image[2,6;3,2;default:mese]"..
	--~ "label[2,15;bla baum\nfoo bar]"..
	--~ "textarea[2,10;5,6;txtarea;Baum;bla]"..
	--~ "item_image_button[2,3;1,1;default:dirt_with_grass;itemimagebutton;ItemImageButton]"..
	--~ "tooltip[0,11;3,2;Blatt;#f00;#000]".."box[0,11;3,2;#00ff00]"..
	--~ "checkbox[2,25;chb;ChB;false]",
	--~ "bgcolor[#aa0a]"..
	--~ "tablecolumns[text,bla]"..
	--~ "table[1,23;3,1;tbl;a,b,c,d;2]"..
	--~ "textlist[0,5;2,0.5;txtlst;a,b,c]"..
	--~ "dropdown[0,6;2;hmdrpdwn;apfel,birne;1]"..
	--~ "tabheader[-1,9;mytabheader;a,b,c;2;false;false]"..
	--~ "background[5,5;1,1;testbg.png;true;10]"..
	""


minetest.register_on_player_receive_fields(function(player, formname, fields)
	if formname ~= "my_formspec" then
		print("other formname: "..formname)
		return
	end
	print(dump(fields))
end)


minetest.register_node("test:node", {
	description = "test node",
	tiles = {"default_wood.png^heart.png"},
	groups = {choppy = 3, oddly_breakable_by_hand = 3},
	on_rightclick = function(pos, node, clicker, itemstack, pointed_thing)
		minetest.show_formspec(clicker:get_player_name(), "my_formspec", my_formspec)
	end,
})

(Note that you might also want to test using style.)

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

The how to test section is intended to be used to show test code. Please test using a mod which demonstrates reordering

@DS-Minetest DS-Minetest marked this pull request as ready for review Aug 2, 2019

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Sorry, done.
(Please remove that WIP.)

@DS-Minetest DS-Minetest referenced a pull request that will close this pull request Aug 2, 2019

@rubenwardy rubenwardy removed the WIP label Aug 2, 2019

@DS-Minetest DS-Minetest force-pushed the DS-Minetest:fs_draw_order branch 2 times, most recently from 4a11713 to b7e292a Aug 3, 2019

@DS-Minetest DS-Minetest changed the title Formspec draw order Make formspec elements real elements for draw order and clipping Aug 3, 2019

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Rebased.

@DS-Minetest DS-Minetest referenced this pull request Aug 3, 2019
19 of 48 tasks complete
src/client/guiscalingfilter.h Outdated Show resolved Hide resolved
src/gui/guiItemImage.h Outdated Show resolved Hide resolved
@rubenwardy
Copy link
Member

left a comment

Nice work.

I'm a little bit confused about the inconsistent use of drop and grab, especially when the element isn't stored in GUIFormspecMenu
It looks like addX methods have one reference - in environment - so don't need to be dropped if not stored but do need to be grabbed if stored.
Things created using new X will have two references - the creator and environment - so need to be dropped if not help

src/gui/guiBackgroundImage.cpp Outdated Show resolved Hide resolved
src/gui/guiBackgroundImage.h Outdated Show resolved Hide resolved
@@ -507,7 +534,8 @@ void GUIFormSpecMenu::parseCheckbox(parserData* data, const std::string &element
Environment->setFocus(e);
}

m_checkboxes.emplace_back(spec,e);
e->grab();

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Aug 8, 2019

Member

Why?

From my understanding, the emplace_back acts as a transfer of ownership

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Aug 8, 2019

Author Contributor

Because of the complaints of stu, all pointers in m_checkboxes, m_inventorylists and co. are dropped when these containers are cleared. The IGUICheckBox * was not made with new but with an add… function. All pointers in m_checkboxes and co. are not only grabbed by the environment but also by the GUIFormSpecMenu code.


GUIItemImage *e = new GUIItemImage(Environment, this, spec.fid,
core::rect<s32>(pos, pos + geom), name, m_font, m_client);
e->drop();

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Aug 8, 2019

Member

other elements allocated in a similar way don't drop themselves. Is this because e isn't stored, and guienvironment is the only reference?

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Aug 8, 2019

Author Contributor

Yes, that's the reason.

);

spec.ftype = f_DropDown;
spec.send = true;

//now really show list
gui::IGUIComboBox *e = Environment->addComboBox(rect, this,spec.fid);
gui::IGUIComboBox *e = Environment->addComboBox(rect, this, spec.fid);

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Aug 8, 2019

Member

this element isn't stored in the formspec, but also isn't dropped. Why does this differ from GUIItemImage?

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Aug 8, 2019

Author Contributor

It is not made with new or create…, but with add…. See http://irrlicht.sourceforge.net/docu/classirr_1_1_i_reference_counted.html#details.

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@rubenwardy

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Buttons have disappeared from the scrollbar:

image

(this is with /formspec on minimal dev test)


Weird text color on error page:

image

probably due to partially transparent box

@rubenwardy rubenwardy added this to Highest priority on top in Formspec PR Priority List [POSSIBLE CONFICTS] Aug 8, 2019

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Thanks for reviewing!

(Huh, I didn't know about that chatcommand. >_<)

The error page seems to be Krock's fault, see #8731 (comment). (I wonder how he did that.)

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

The scrollbar buttons aren't there because they are set to not visible here:

bool visible = (border_size != 0);
up_button->setVisible(visible);
down_button->setVisible(visible);

with this border_size:
border_size = RelativeRect.getWidth() < h * 4 ? 0 : h;

(The scrollbar is not long enough relative to it's wideness. This is an intended feature.)

This will happen in other PRs that use the new scrollbar, too, like #8530.

Related: #8507

Notice in the second screenshot that the up/down arrow buttons are now automatically hidden when there is not enough space to fully accommodate them.

Setting whether there should be buttons is for #8530 (or a later PR).

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

box is the one element that commonly overlaps, I could use sendToBack on boxes if there's not real_coordinates.

@DS-Minetest DS-Minetest force-pushed the DS-Minetest:fs_draw_order branch from df238a9 to ae560ac Aug 8, 2019

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Rebased, the error window error is fixed.

@rubenwardy

This comment has been minimized.

@Linuxdirk

This comment has been minimized.

Copy link

commented Sep 10, 2019

Just a quick question. Does it fix this?

#8036

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Just a quick question. Does it fix this?

Yes, for everything except the rendering of items in inventory lists, as that is done last. This will probably change in future

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

As #8036 is probably mostly fixed after this PR (except of with combination with inventory lists, which will make it a duplicate of #4844), I'll add an automatic close thing to this PR.

@Linuxdirk

This comment has been minimized.

Copy link

commented Sep 10, 2019

mostly fixed after this PR (except of with combination with inventory lists

Since the combination with inventory lists is the only thing mentioned in said issue it is not fixed at all then? 😄

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

mostly fixed after this PR (except of with combination with inventory lists

Since the combination with inventory lists is the only thing mentioned in said issue it is not fixed at all then? smile

Well, the title says "behind other elements".
That inventory lists are drawn first is already covered in #8036. Please, don't say, that your issue is a duplicate, we need to reach the status of being a game with a thousand issues. 🙂

@rubenwardy rubenwardy added this to the 5.1.0 milestone Sep 15, 2019

@rubenwardy rubenwardy self-requested a review Sep 15, 2019

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Segfault in main menu

0x00007ffff66ddda8 in __cxxabiv1::__dynamic_cast (src_ptr=0x5555564cede0, 
    src_type=0x555555d68438 <typeinfo for irr::gui::IGUIElement>, 
    dst_type=0x555555d6a300 <typeinfo for GUIBox>, src2dst=0)
    at /build/gcc/src/gcc/libstdc++-v3/libsupc++/dyncast.cc:68
68	/build/gcc/src/gcc/libstdc++-v3/libsupc++/dyncast.cc: No such file or directory.
(gdb) bt full
#0  0x00007ffff66ddda8 in __cxxabiv1::__dynamic_cast (src_ptr=0x5555564cede0, 
    src_type=0x555555d68438 <typeinfo for irr::gui::IGUIElement>, 
    dst_type=0x555555d6a300 <typeinfo for GUIBox>, src2dst=0)
    at /build/gcc/src/gcc/libstdc++-v3/libsupc++/dyncast.cc:68
        vtable = 0x7ffff7d566a8 <vtable for irr::gui::CGUITabControl+32>
        prefix = 0x7ffff7d56698 <vtable for irr::gui::CGUITabControl+16>
        whole_ptr = 0x5555564cede0
        whole_type = 0x0
        result = {dst_ptr = 0x0, 
          whole2dst = __cxxabiv1::__class_type_info::__unknown, 
          whole2src = __cxxabiv1::__class_type_info::__unknown, 
          dst2src = __cxxabiv1::__class_type_info::__unknown, 
          whole_details = 16}
        whole_vtable = 0x7ffff7d566a8 <vtable for irr::gui::CGUITabControl+32>
        whole_prefix = 0x7ffff7d56698 <vtable for irr::gui::CGUITabControl+16>
#1  0x000055555587df10 in GUIFormSpecMenu::legacySortElements(irr::core::list<irr::gui::IGUIElement*>::Iterator) ()
No symbol table info available.
#2  0x00005555558a2a57 in GUIFormSpecMenu::regenerateGui(irr::core::vector2d<unsigned int>) ()
No symbol table info available.
#3  0x0000555555896f86 in GUIFormSpecMenu::drawMenu() ()
No symbol table info available.
--Type <RET> for more, q to quit, c to continue without paging--
#4  0x00005555558ef128 in GUIModalMenu::draw() ()
No symbol table info available.
#5  0x00007ffff7bc7142 in irr::gui::CGUIStaticText::draw() () from /usr/lib/libIrrlicht.so.1.8
No symbol table info available.
#6  0x00007ffff7b73d5a in irr::gui::CGUIEnvironment::drawAll() () from /usr/lib/libIrrlicht.so.1.8
No symbol table info available.
#7  0x000055555587afb1 in GUIEngine::run() ()
No symbol table info available.
#8  0x000055555587bb30 in GUIEngine::GUIEngine(JoystickController*, irr::gui::IGUIElement*, IMenuManager*, MainMenuData*, bool&) ()
No symbol table info available.
#9  0x00005555557208b8 in ClientLauncher::main_menu(MainMenuData*) ()
No symbol table info available.
#10 0x00005555557217b3 in ClientLauncher::launch_game(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, bool, GameParams&, Settings const&) ()
No symbol table info available.
#11 0x0000555555723d12 in ClientLauncher::run(GameParams&, Settings const&) ()
No symbol table info available.
#12 0x00005555556f85e9 in main ()
No symbol table info available.
@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

The rect of inventory lists is now calculated correctly:
screenshot_20190915_185104
Thanks for testing!

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Here's a stack trace of the segfault with debug symbols

Thread 1 "minetest" received signal SIGSEGV, Segmentation fault.
irr::core::list<irr::gui::IGUIElement*>::Iterator::operator++ (this=0x7fffffffc9f8) at /usr/include/irrlicht/irrList.h:45
45			Iterator  operator ++(s32) { Iterator tmp = *this; Current = Current->Next; return tmp; }
(gdb) bt full
#0  irr::core::list<irr::gui::IGUIElement*>::Iterator::operator++ (this=0x7fffffffc9f8) at /usr/include/irrlicht/irrList.h:45
        tmp = {Current = 0x0}
#1  0x0000555555d60968 in GUIFormSpecMenu::regenerateGui (this=0x555556b402d0, screensize=...) at /home/ruben/dev/minetest/src/gui/guiFormSpecMenu.cpp:2878
        mydata = {explicit_size = false, real_coordinates = false, invsize = {X = 0, Y = 0}, size = {X = 100, Y = 100}, offset = {X = 0.5, Y = 0.5}, anchor = {X = 0.5, Y = 0.5}, rect = {
            UpperLeftCorner = {X = 0, Y = 0}, LowerRightCorner = {X = 0, Y = 0}}, basepos = {X = 139, Y = 145}, screensize = {X = 858, Y = 591}, focused_fieldname = "", 
          table_options = std::vector of length 0, capacity 0, table_columns = std::vector of length 0, capacity 0, table_dyndata = std::unordered_map with 0 elements}
        focused_element = 0x555556b402d0
        __PRETTY_FUNCTION__ = "virtual void GUIFormSpecMenu::regenerateGui(v2u32)"
        elements = std::vector of length 1, capacity 1 = {""}
        i = 1
        enable_prepends = true
        skin = 0x55555694c180
        old_font = 0x555556b295a0
        legacy_sort_start = {Current = 0x0}
#2  0x0000555555da9749 in GUIModalMenu::draw (this=0x555556b402d0) at /home/ruben/dev/minetest/src/gui/modalMenu.cpp:76
        driver = 0x5555568d6520
        screensize = {X = 858, Y = 591}
#3  0x00007ffff7bc7142 in irr::gui::CGUIStaticText::draw() () from /usr/lib/libIrrlicht.so.1.8
No symbol table info available.
#4  0x00007ffff7b73d5a in irr::gui::CGUIEnvironment::drawAll() () from /usr/lib/libIrrlicht.so.1.8
No symbol table info available.
#5  0x0000555555d47390 in GUIEngine::run (this=0x7fffffffd070) at /home/ruben/dev/minetest/src/gui/guiEngine.cpp:296
        current_screen_size = @0x5555568d6620: {Width = 858, Height = 591}
        driver = 0x5555568d6520
        text_height = 20
        previous_screen_size = {Width = 858, Height = 591}
        sky_color = {color = 4287412986}
#6  0x0000555555d467be in GUIEngine::GUIEngine (this=0x7fffffffd070, joystick=0x555556b268c8, parent=0x555556a30f10, menumgr=0x55555646fc60 <g_menumgr>, data=0x7fffffffd310, 
    kill=@0x5555564a2800: false) at /home/ruben/dev/minetest/src/gui/guiEngine.cpp:196
        soundfetcher = {<OnDemandSoundFetcher> = {_vptr.OnDemandSoundFetcher = 0x5555563ff668 <vtable for MenuMusicFetcher+16>}, m_fetched = std::set with 1 element = {[0] = "main_menu"}}
        rect = {UpperLeftCorner = {X = 4, Y = 0}, LowerRightCorner = {X = 4, Y = 20}}
        soundfetcher = <optimized out>
        rect = <optimized out>
        texture = <optimized out>
        __for_range = <optimized out>
        __for_begin = <optimized out>
        __for_end = <optimized out>
        e = <optimized out>
#7  0x0000555555bf2d6d in ClientLauncher::main_menu (this=0x7fffffffdbe0, menudata=0x7fffffffd310) at /home/ruben/dev/minetest/src/client/clientlauncher.cpp:560
        kill = 0x5555564a2800 <porting::g_killed>
        driver = 0x5555568d6520
        mymenu = {_vptr.GUIEngine = 0x5555563ff648 <vtable for GUIEngine+16>, m_parent = 0x555556a30f10, m_menumanager = 0x55555646fc60 <g_menumgr>, m_smgr = 0x555556b1b0f0, 
          m_data = 0x7fffffffd310, m_texture_source = 0x555556b3fc50, m_sound_manager = 0x555556b400e0, m_formspecgui = 0x555556b3fbd0, m_buttonhandler = 0x555556b400a0, 
          m_menu = 0x555556b402d0, m_kill = @0x5555564a2800, m_startgame = false, m_script = 0x555556b3a430, m_scriptdir = "/home/ruben/dev/minetest/bin/../builtin/mainmenu", m_textures = {
            {texture = 0x0, tile = false, minsize = 0}, {texture = 0x0, tile = 128, minsize = 32767}, {texture = 0x555556b28b90, tile = false, minsize = 16}, {texture = 0x0, tile = 192, 
              minsize = 32767}}, m_irr_toplefttext = 0x555556a092b0, m_toplefttext = {m_string = L"Minetest Game", m_colors = std::vector of length 13, capacity 16 = {{color = 4294967295}, 
              {color = 4294967295}, {color = 4294967295}, {color = 4294967295}, {color = 4294967295}, {color = 4294967295}, {color = 4294967295}, {color = 4294967295}, {
                color = 4294967295}, {color = 4294967295}, {color = 4294967295}, {color = 4294967295}, {color = 4294967295}}, m_has_background = false, m_background = {color = 0}}, 
          m_clouds_enabled = true, m_cloud = {dtime = 0.0340000018, lasttime = 193, clouds = 0x555556509a90, camera = 0x5555569d80d0}}
#8  0x0000555555bf1e00 in ClientLauncher::launch_game (this=0x7fffffffdbe0, error_message="", reconnect_requested=false, game_params=..., cmd_args=...)
    at /home/ruben/dev/minetest/src/client/clientlauncher.cpp:424
        newport = 32767
        worldspecs = std::vector of length -35394917066724837, capacity -87413170238280657 = {<error reading variable worldspecs (Cannot access memory at address 0x74752e42475f6e65)>
        menudata = {servername = "", serverdescription = "", address = "", port = "30000", name = "rubenwardy", password = "", do_reconnect = false, selected_world = 0, 
          simple_singleplayer_mode = false, script_data = {reconnect_requested = false, errormessage = ""}}
--Type <RET> for more, q to quit, c to continue without paging--
        __PRETTY_FUNCTION__ = "bool ClientLauncher::launch_game(std::string&, bool, GameParams&, const Settings&)"
#9  0x0000555555bf0654 in ClientLauncher::run (this=0x7fffffffdbe0, game_params=..., cmd_args=...) at /home/ruben/dev/minetest/src/client/clientlauncher.cpp:218
        game_has_run = false
        text = 0x555556b3ffb0 L"\x55ee4310啕\x55eb2506啕\x55ee05e8啕\x61754c\x656c61\060"
        __PRETTY_FUNCTION__ = "bool ClientLauncher::run(GameParams&, const Settings&)"
        camera = 0x555556b33d40
        chat_backend = {m_console_buffer = {m_scrollback = 500, m_unformatted = std::vector of length 0, capacity 0, m_cols = 0, m_rows = 0, m_scroll = 0, 
            m_formatted = std::vector of length 0, capacity 0, m_empty_formatted_line = {fragments = std::vector of length 0, capacity 0, first = true}}, m_recent_buffer = {
            m_scrollback = 6, m_unformatted = std::vector of length 0, capacity 0, m_cols = 0, m_rows = 0, m_scroll = 0, m_formatted = std::vector of length 0, capacity 0, 
            m_empty_formatted_line = {fragments = std::vector of length 0, capacity 0, first = true}}, m_prompt = {m_prompt = L"]", m_line = L"", 
            m_history = std::vector of length 0, capacity 0, m_history_index = 0, m_history_limit = 500, m_cols = 0, m_view = 0, m_cursor = 0, m_cursor_len = 0, 
            m_nick_completion_start = 0, m_nick_completion_end = 0}}
        error_message = ""
        reconnect_requested = false
        first_loop = true
        retval = true
        kill = 0x5555564a2800 <porting::g_killed>
#10 0x000055555602230b in main (argc=1, argv=0x7fffffffdf58) at /home/ruben/dev/minetest/src/main.cpp:217
        retval = 21845
        cmd_args = {m_settings = std::unordered_map with 0 elements, m_defaults = std::unordered_map with 0 elements, m_callbacks = std::unordered_map with 0 elements, 
          m_callback_mutex = {<std::__mutex_base> = {_M_mutex = {__data = {__lock = 0, __count = 0, __owner = 0, __nusers = 0, __kind = 0, __spins = 0, __elision = 0, __list = {
                    __prev = 0x0, __next = 0x0}}, __size = '\000' <repeats 39 times>, __align = 0}}, <No data fields>}, m_mutex = {<std::__mutex_base> = {_M_mutex = {__data = {__lock = 0, 
                  __count = 0, __owner = 0, __nusers = 0, __kind = 0, __spins = 0, __elision = 0, __list = {__prev = 0x0, __next = 0x0}}, __size = '\000' <repeats 39 times>, 
                __align = 0}}, <No data fields>}}
        cmd_args_ok = true
        game_params = {socket_port = 30000, world_path = "/home/ruben/dev/minetest/bin/../worlds/world", game_spec = {id = "minetest", name = "Minetest Game", author = "Minetest", 
            release = 0, path = "/home/ruben/dev/minetest/bin/../games/minetest_game", gamemods_path = "/home/ruben/dev/minetest/bin/../games/minetest_game/mods", 
            addon_mods_paths = std::set with 1 element = {[0] = "/home/ruben/dev/minetest/bin/../mods"}, 
            menuicon_path = "/home/ruben/dev/minetest/bin/../games/minetest_game/menu/icon.png"}, is_dedicated_server = false}
        isServer = false
        __PRETTY_FUNCTION__ = "int main(int, char**)"
        launcher = {list_video_modes = false, skip_main_menu = false, use_freetype = true, random_input = false, address = "", playername = "rubenwardy", password = "", 
          input = 0x555556b268c0, receiver = 0x7fffe800bbd0, skin = 0x55555694c180, font = 0x0, gamespec = {id = "", name = "", author = "", release = 0, path = "", gamemods_path = "", 
            addon_mods_paths = std::set with 0 elements, menuicon_path = ""}, worldspec = {path = "/home/ruben/dev/minetest/bin/../worlds/world", name = "[--world parameter] [new]", 
            gameid = "minetest"}, simple_singleplayer_mode = false, current_playername = "inv£lid", current_password = "", current_address = "does-not-exist", current_port = 0}
@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

The bugs should be fixed now.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Tested with Lavaland, quickly.

Noticed that I was unable to use the [List] [Ok] buttons or type a name in the field on this formspec (beds)
screenshot_20190916_045511 The other buttons and checkbox work, however. (Here's the code, for your reference: https://github.com/jastevenson303/Lavaland/blob/master/mods/beds/functions.lua#L328)

Formspecs look different? A regular field:
screenshot_20190916_045451

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Thanks for testing, @jastevenson303!
The first bug should actually already be fixed by 6055a5f. Did you test with the newest version of this PR?
I can reproduce the second thing and will look into it.

@ClobberXD
Copy link
Contributor

left a comment

It's great to see someone working on such large PRs. Nice! 👍

I can't understand most of the code, and I've just pointed out some code-style inconsistencies.

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/network/networkprotocol.h Outdated Show resolved Hide resolved
@@ -0,0 +1,37 @@
/*
Part of Minetest

This comment has been minimized.

Copy link
@ClobberXD

ClobberXD Sep 16, 2019

Contributor

Why part of? Other files just have Minetest.

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Sep 16, 2019

Author Contributor

Afaik, it means that the file is part of minetest.

See also #8740 (comment).

This comment has been minimized.

Copy link
@ClobberXD

ClobberXD Sep 17, 2019

Contributor

All files are part of Minetest tho. Also, the linked comment says nothing about adding "Part of"

return;

Environment->getVideoDriver()->draw2DRectangle(
m_color, AbsoluteRect, &AbsoluteClippingRect);

This comment has been minimized.

Copy link
@ClobberXD

ClobberXD Sep 16, 2019

Contributor

Extra indent

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Sep 16, 2019

Author Contributor

Afaik, it's allowed.

src/gui/guiFormSpecMenu.cpp Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

The simple field is now fixed.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

This is with ad4b58d @DS-Minetest

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@jastevenson303 I think, the problem is that you have set the height to 4 but the inventory only has a size to allow a height of 3. Therefor the list overlaps with invisible slots.
I'd suggest to change the 4 here to a 3.

I'm not sure how I could fix such things.

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Thanks for the code style review @ClobberXD!

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

Ok, I've fixed your bug, @jastevenson303.
In your particular situation, it is a bug caused by you, evil you. But there might be many people who do the same mistake (because of lazy copying). Because of this I've fixed this.
Also, now there won't be warnings anymore in the draw function if the inventory or the list does not exist (in most cases; there might still be warnings if the inventory somehow disappears). (The warnings were like torturous screams of horror.)

@DS-Minetest DS-Minetest force-pushed the DS-Minetest:fs_draw_order branch from 5fcb14b to a594a9d Sep 17, 2019

@rubenwardy
Copy link
Member

left a comment

I've been running this for the last few days, and not found any issues

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

Thanks!

(The last commit has no functional changes.)

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Show resolved Hide resolved
{
IGUIElement* e = getElementFromId(s.fid);
if(e != NULL) {
else {

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Sep 18, 2019

Member

} else {

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Sep 18, 2019

Author Contributor

This is adapted to the code style of the whole if else construct here. I would have to change it everywhere.
(Also it's quite readable.)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Sep 18, 2019

Member

Only needed for touched lines

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Show resolved Hide resolved

@DS-Minetest DS-Minetest force-pushed the DS-Minetest:fs_draw_order branch from 145f26f to dbcf6c1 Sep 18, 2019

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Rebased.

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Luckily the bgcolor fix PR's drawback has shown me a bug here. Fixed.
(The size of the rect of the form is not given in prepends.)

@DS-Minetest

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

override added. See also #8957.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.